-
Notifications
You must be signed in to change notification settings - Fork 13
fix: count pdisk-vdisk column width for skeletons #2214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the counting of pdisk-vdisk column widths by changing the types and conversions for maximumSlotsPerDisk and maximumDisksPerNode from strings to numbers, and propagating these values into table column settings for consistent styling. Key changes include:
- Updated functions in storage/utils.ts and types.ts to use numbers instead of strings for disk slot and disk count.
- Modified component interfaces in storage and paginated table files to accept and forward the new columnSettings, along with propagating tableStyle for layout updates.
- Refinements in the display components to adjust for the centralized application of CSS variables.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/store/reducers/storage/utils.ts | Changed parameter and return types for maximumSlotsPerDisk/maximumDisksPerNode and applied Number() conversion |
src/store/reducers/storage/types.ts | Updated type definitions to reflect numeric values for MaximumSlotsPerDisk/MaximumDisksPerNode and added columnSettings |
src/containers/Storage/StorageNodes/getNodes.ts | Included forwarding of columnSettings in the data response |
src/containers/Storage/StorageNodes/columns/columns.tsx | Removed inline style variables and usage for pdisk columns to support centralized styling |
src/containers/Storage/StorageNodes/PaginatedStorageNodesTable.tsx, src/containers/Storage/PaginatedStorageNodes.tsx | Added/forwarded tableStyle and onDataFetched props |
src/components/PaginatedTable/* | Updated types and callbacks to support the new column settings and tableStyle propagation |
Files not reviewed (1)
- src/containers/Storage/StorageNodes/columns/StorageNodesColumns.scss: Language not supported
Comments suppressed due to low confidence (1)
src/containers/Storage/StorageNodes/columns/columns.tsx:30
- Verify that removing the inline pDiskStyles and CSS variable definitions is intentional; ensure that the centralized styling applied in higher-level components correctly sets these CSS variables for proper pdisk-vdisk column width display.
const MAX_SLOTS_CSS_VAR = '--maximum-slots';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the storage utilities to count PDisk–VDisk column widths more accurately by converting maximum slot and disk values from strings to numbers. It also propagates these numeric values through to the table response and component styling while updating corresponding tests.
- Change maximumSlotsPerDisk and maximumDisksPerNode types in utils and types from string to number.
- Update tests to expect numeric values.
- Add and propagate new columnSettings and tableStyle properties for proper styling.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/store/reducers/storage/utils.ts | Updated parameter types for maximum slots/disks and their usage. |
src/store/reducers/storage/types.ts | Changed storage node type properties from string to number. |
src/store/reducers/storage/tests/*.test.ts | Updated test expectations to reflect numeric values. |
src/containers/Storage/StorageNodes/getNodes.ts | Propagates new columnSettings from the response. |
src/containers/Storage/StorageNodes/columns/columns.tsx | Removed inline CSS style settings for PDisk columns. |
src/containers/Storage/StorageNodes/PaginatedStorageNodesTable.tsx | Added tableStyle and onDataFetched handling to pass through styling. |
src/containers/Storage/PaginatedStorageNodes.tsx | Updated component to use tableStyle state with dynamic CSS variables. |
src/components/PaginatedTable/* | Updated types and callbacks to work with the new columnSettings/tableStyle. |
Files not reviewed (1)
- src/containers/Storage/StorageNodes/columns/StorageNodesColumns.scss: Language not supported
Comments suppressed due to low confidence (2)
src/containers/Storage/PaginatedStorageNodes.tsx:88
- Ensure that numeric values used for CSS custom properties are formatted correctly (e.g., appending units if necessary) to avoid styling issues in the rendered table.
setTableStyle({ [MAX_SLOTS_CSS_VAR]: maxSlotsPerDisk, [MAX_DISKS_CSS_VAR]: maxDisksPerNode });
src/containers/Storage/StorageNodes/columns/columns.tsx:40
- Inline style handling for the PDisk column was removed; ensure that the new styling via tableStyle is correctly applied and that any required unit conversion for CSS custom properties is handled downstream.
<div className={b('pdisks-wrapper')}>
0c7d78c
to
af31a19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
43042a7
to
5ef9384
Compare
Closes #2212
Stand
The idea of this pr is that we want to set row skeleton width for pdisks based on the info that we received from backend on fetching data chunk.
So that on fetching datachunk we set these variables in table state and pass them as custom table style - other calculations are done in css for the whole table (skeletons and pdisks)
Previously css variables were set for every pdisk column - now it is set to the whole table
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: ✅
Current: 83.37 MB | Main: 83.37 MB
Diff: +0.60 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information