fix(Cluster): side paddings #2301
Conversation
src/containers/Nodes/Nodes.scss
Outdated
| --nodes-side-padding: var(--g-spacing-5); | ||
| width: calc(100% + var(--nodes-side-padding)); | ||
| padding-right: var(--nodes-side-padding); |
There was a problem hiding this comment.
Why don't you just remove padding here?
Such approach (100%+ width, hidden paddings, negative margins, transforms) is cursed, it causes unexpected behavior and bugs long-term - you fix cluster controls and autorefresh layout bugs every 2-3 weeks
There was a problem hiding this comment.
Really, thats time to get rid of this evil.
So, I've done it. Stand redeployed.
There was a problem hiding this comment.
You didn't delete groups wrapper classes
src/containers/Nodes/Nodes.scss
Outdated
| --nodes-side-padding: var(--g-spacing-5); | ||
| width: calc(100% + var(--nodes-side-padding)); | ||
| padding-right: var(--nodes-side-padding); |
There was a problem hiding this comment.
You didn't delete groups wrapper classes
| } | ||
|
|
||
| &__content { | ||
| width: calc(100% - var(--cluster-side-padding)); |
There was a problem hiding this comment.
It seems that this is the last evil, but essential. For now our scroll container doesn't have paddings, so sticky elements stick to it.
I found this solution better than passing to all tables inside Cluster page classes with left: 20px
There was a problem hiding this comment.
What if you add left padding to cluster scroll container, and right padding to all cluster components (dashboard, tabs, tabs content)?
There was a problem hiding this comment.
What if you add left padding to cluster scroll container, and right padding to all cluster components (dashboard, tabs, tabs content)?
Than we get back with layout bug on the left from tabs :(
There was a problem hiding this comment.
Pull Request Overview
This PR adjusts the side paddings and sticky positioning across various cluster-related components. Key changes include:
- Removal of the groups wrapper padding in Storage and Nodes containers.
- Updates to sticky behavior and side padding in Cluster and ClusterOverview components, including new wrapper elements.
- A width change from 100% to max-content in TableWithControlsLayout controls.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/containers/Storage/Storage.scss | Removed redundant groups wrapper padding. |
| src/containers/Nodes/Nodes.scss | Removed redundant groups wrapper padding. |
| src/containers/Cluster/ClusterOverview/ClusterOverview.scss | Removed sticky positioning from the overview wrapper. |
| src/containers/Cluster/Cluster.tsx | Added new dashboard and content wrappers; adjusted route path formatting. |
| src/containers/Cluster/Cluster.scss | Updated styling for side paddings and sticky elements. |
| src/components/TableWithControlsLayout/TableWithControlsLayout.scss | Changed controls width from full-width to max-content. |
Comments suppressed due to low confidence (2)
src/containers/Cluster/ClusterOverview/ClusterOverview.scss:32
- Removal of the sticky positioning from the __overview-wrapper class may affect the intended behavior. If sticky layout is required for proper usability, please revisit this change.
position: sticky; (removed) and left: 0; (removed)
src/components/TableWithControlsLayout/TableWithControlsLayout.scss:23
- Changing the width from 100% to 'max-content' can impact responsiveness. Verify that this adjustment does not lead to layout overflow issues across different screen sizes.
width: max-content;
|
| @@ -20,7 +20,7 @@ | |||
| &__controls { | |||
| z-index: 3; | |||
|
|
|||
| width: 100%; | |||
| width: max-content; | |||
There was a problem hiding this comment.
width: 100% here is for the create db button - it should stick to the right
Expected:
You code:
Closes #2300
Stand
CI Results
Test Status: ❌ FAILED
📊 Full Report
😟 No changes in tests. 😕
Bundle Size:⚠️
Current: 0.00 KB | Main: 0.00 KB
Diff: 0.00 KB (N/A)
ℹ️ CI Information