Skip to content
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.

Sign up for GitHub

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

fix: Fixes editable table cell paddings for certain corner cases #3145

Merged
merged 3 commits into from
Dec 23, 2024
Merged

fix: Fixes editable table cell paddings for certain corner cases #3145

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cff76e
fix editable cell bottom padding
pan-kot Dec 17, 2024
b4a3b82
fix editable + sticky padding
pan-kot Dec 17, 2024
261c55f
fix first editable col shaking
pan-kot Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions src/table/body-cell/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,10 @@ $cell-negative-space-vertical: 2px;
text-align: start;

@include cell-padding-inline-start($cell-horizontal-padding);
@include cell-padding-inline-end($cell-horizontal-padding);

@include cell-padding-block-start($cell-vertical-padding);
@include cell-padding-block-end($cell-vertical-padding-w-border);
@include cell-padding-inline-end($cell-horizontal-padding);

&-content {
box-sizing: border-box;
Expand All @@ -147,28 +148,21 @@ $cell-negative-space-vertical: 2px;
}
&:first-child {
border-inline-start: $border-placeholder;
@include cell-padding-inline-start($cell-edge-horizontal-padding);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from first-child:not(.is-visual-refresh). There is no reason to include the ":not(.is-visual-refresh)" because first column padding for VR is overridden anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the :not() pseudo-selectors are good for clarity and for reducing overrides, and moreover these ones in particular help separate the "non-VR" styles now that VR is the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this looks like an if-else statements where if and else parts are also set apart. I think it is cleaner when some style is set as default and then another one overrides it conditionally.

I do, however, agree that VR can be the new default now. I can take a follow-up task to rework these styles so that instead of passing is-visual-refresh we will instead pass is-classic and the overrides are inverted.

}
&:last-child {
border-inline-end: $border-placeholder;
@include cell-padding-inline-end($cell-edge-horizontal-padding);
}
// Using very small padding for 1st column cells in VR.
&.is-visual-refresh:first-child {
&:not(.has-striped-rows) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed "&:not(.has-striped-rows)" because there is an explicit override for ".has-striped-rows" down below.

@include cell-padding-inline-start(awsui.$space-xxxs);
&:not(.body-cell-edit-active).body-cell-editable:hover {
@include cell-padding-inline-start(calc(#{awsui.$space-xxxs} + #{awsui.$border-divider-list-width}));
}
}

&.sticky-cell-pad-inline-start:not(.has-selection) {
@include cell-padding-inline-start($cell-horizontal-padding);
@include cell-padding-inline-start(awsui.$space-xxxs);
&.body-cell-editable:hover {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the ":not(.body-cell-edit-active)" prefix because the active edit cell cannot be hovered anyways. This also fixes the bug when the wrong padding was assigned for sticky + editable + hovered cells.

@include cell-padding-inline-start(calc(#{awsui.$space-xxxs} + #{awsui.$border-divider-list-width}));
}

/*
Striped rows requires additional left padding because the
shaded background makes the child content appear too close
to the table edge.
*/
// Using slightly larger padding for tables with striped rows because the shaded background
// makes the child content appear too close to the table edge.
&:first-child.has-striped-rows {
@include cell-padding-inline-start(awsui.$space-xxs);

Expand All @@ -177,6 +171,11 @@ $cell-negative-space-vertical: 2px;
}
}

// Using normal padding when 1st column is sticky.
&.sticky-cell-pad-inline-start:not(.has-selection) {
@include cell-padding-inline-start($cell-horizontal-padding);
}

/*
Remove the placeholder border if the row is not selectable.
Rows that are not selectable will reserve the horizontal space
Expand All @@ -186,9 +185,6 @@ $cell-negative-space-vertical: 2px;
border-inline-start: none;
}
}
&:first-child:not(.is-visual-refresh) {
@include cell-padding-inline-start($cell-edge-horizontal-padding);
}
&-first-row {
border-block-start: $border-placeholder;
}
Expand Down Expand Up @@ -246,9 +242,6 @@ $cell-negative-space-vertical: 2px;
transition-duration: awsui.$motion-duration-transition-show-quick;
transition-timing-function: awsui.$motion-easing-sticky;
}
&-pad-left:not(.has-selection):not(.is-visual-refresh.body-cell:first-child.has-striped-rows) {
@include cell-padding-inline-start(awsui.$space-table-horizontal);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this completely. The table cells should use normal paddings also when sticky.

}
&.body-cell-shaded {
background: awsui.$color-background-cell-shaded;
}
Expand Down Expand Up @@ -418,7 +411,8 @@ $cell-negative-space-vertical: 2px;
}
@include cell-padding-inline-start($editing-cell-padding-inline);
@include cell-padding-inline-end($editing-cell-padding-inline);
@include cell-padding-block($editing-cell-padding-block);
@include cell-padding-block-start($editing-cell-padding-block);
@include cell-padding-block-end(calc($editing-cell-padding-block + 1px));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a small extra space here ensures better input alignment especially in compact mode.

}

&:not(.body-cell-edit-active) {
Expand Down
Loading