fix: Fixes table editable cell styles and simplifies it #3061

pan-kot · 2024-11-26T14:19:07Z

Description

The PR refactors table body cell styles with the focus on editable cells. That is needed for the follow-up task to move editable cell style from the cell element to the cell content instead. This would allow having both row expand toggle and editable cell trigger in a single cell (in 1st column).

How has this been tested?

Screenshot tests
Manual testing on the cell-permutations page

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Changes include appropriate documentation updates.
Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
Changes do not include unsupported browser features, see CONTRIBUTING.md.
Changes were manually tested for accessibility, see accessibility guidelines.

Security

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

Changes are covered with new/existing unit tests?
Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov · 2024-11-26T14:33:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.35%. Comparing base (606beea) to head (9836e89).
Report is 36 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3061   +/-   ##
=======================================
  Coverage   96.34%   96.35%           
=======================================
  Files         778      780    +2     
  Lines       21908    21971   +63     
  Branches     7507     7472   -35     
=======================================
+ Hits        21108    21170   +62     
- Misses        748      749    +1     
  Partials       52       52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pan-kot · 2024-12-03T12:54:23Z

src/table/body-cell/styles.scss

 $cell-offset: calc(#{awsui.$space-m} + #{awsui.$space-xs});

-// Ensuring enough space for absolute-positioned focus outlines of focus-able cell content elements.
-$cell-negative-space-vertical: 2px;


This negative space was used to virtually enlarge the cell-content element. That is because the element has overflow: hidden but when there are interactive elements inside such a link or a button dropdown - the focus outline is being cut off.

The negative space is no longer needed because now the cell content size matches cell size. This way the outline can use the available cell space while the content is still truncated correctly.

Update: the negative space is still needed at least for the compact mode.

pan-kot · 2024-12-03T12:55:09Z

src/table/body-cell/styles.scss

+    align-content: center;
+
+    &.body-cell-align-top {
+      align-content: baseline;


Now the cell content takes the entire cell size so the alignment must be attached to it instead of the cell.

pan-kot · 2024-12-03T12:55:45Z

src/table/body-cell/styles.scss

+      align-content: baseline;
+    }
+
+    &:not(.body-cell-wrap) {


The .body-cell-wrap only takes effect on the cell-content element so attaching the style to it directly.

pan-kot · 2024-12-03T12:57:26Z

src/table/body-cell/styles.scss

-    // That is to prevent focus outline on cell content from being cut out.
-    > .body-cell-content {
-      padding-inline-start: awsui.$border-divider-list-width;
-      margin-inline-start: calc(-1 * #{awsui.$border-divider-list-width});


This negative space shenanigans are no longer needed because there is plenty of space for the focus outline.

pan-kot · 2024-12-03T13:00:48Z

src/table/body-cell/styles.scss

@@ -380,19 +373,6 @@ $cell-negative-space-vertical: 2px;
      color: awsui.$color-text-button-normal-active;
    }

-    &-form {


For editing cells we used overflow: visible on cell content together with negative margins so that the editor can take more space inside a cell and its focus outline is not cut off. Instead, in the refactored versions we just use different paddings for editing cells and there is no longer an issue with the outline.

Update: the overflow visible is still needed for editable content that includes dropdowns. I will contribute a new screenshot test to capture that.

pan-kot · 2024-12-03T13:02:58Z

src/table/body-cell/td-element.tsx

@@ -90,6 +90,7 @@ export const TableTdElement = React.forwardRef<HTMLTableCellElement, TableTdElem
      collapseButtonLabel,
      verticalAlign,
      resizableColumns,
+      resizableStyle,


The resizableStyle replaces style to make the cell API better predictable so that no unexpected attributes can be assigned from the outside.

pan-kot · 2024-12-03T13:05:29Z

src/table/styles.scss

@@ -53,6 +53,7 @@

 .table {
  inline-size: 100%;
+  block-size: 100%;


This change does not affect the table itself - the size of the table is determined by the size of its content.

However, the table element needs the height set for cells height to take an effect, see discussion: https://stackoverflow.com/questions/3215553/make-a-div-fill-an-entire-table-cell.

pan-kot · 2024-12-03T13:05:50Z

src/table/body-cell/styles.scss

-    white-space: nowrap;
-    overflow: hidden;
-    text-overflow: ellipsis;
+    block-size: 100%;


Making cell content span to the entire cell height.

georgylobko · 2024-12-09T11:16:01Z

src/table/body-cell/td-element.tsx

@@ -101,6 +102,8 @@ export const TableTdElement = React.forwardRef<HTMLTableCellElement, TableTdElem
    const Element = isRowHeader ? 'th' : 'td';
    const isVisualRefresh = useVisualRefresh();

+    resizableStyle = resizableColumns ? {} : resizableStyle;


this condition looks counterintuitive. you apply resizableStyle only when resizableColumns is false. why is that?

That is taken as is from the current impl: https://github.com/cloudscape-design/components/blob/main/src/table/internal.tsx#L627

When resizable columns is on, the styles are assigned directly to cell nodes in src/table/column-widths-utils.ts and I think the properties like min-width and max-width are ignored.

I think there are further opportunities for refactoring and maybe this condition can be just removed but I'd not include this into the current PR.

YueyingLu · 2024-12-09T14:35:40Z

Found 2 issues with manual testing:

can not open select dropdown

Screen.Recording.2024-12-09.at.22.31.19.mov

the input position seems a bit off

pan-kot · 2024-12-09T15:15:29Z

@georgylobko, @YueyingLu

I added a new commit with the following changes:

Reverted changes to negative vertical space. That is because in classic+compact the focus outline can be still cut off if the button is rendered inside content and the rows only have text content. The vertical paddings we use for table cell are under 2px whilst the focus outline offset for buttons is 2px - hence we need to do a trick so that the focus outline can fit.
Reverted changes to visible overflow in editable cells. When a cell is editing the overflow is allowed so that components with dropdown like Select can be displayed correctly.

I will re-run the screenshot tests for these changes, too.

…al margin

pan-kot added 2 commits November 26, 2024 13:23

move cell wrap style to cell content

4c9b8a8

replaces cell style with resizable-style

e04b439

pan-kot force-pushed the editable-cells-wrapper branch from 4de7d75 to 0d64af7 Compare November 26, 2024 14:19

pan-kot deployed to dev-pages November 26, 2024 14:26 — with GitHub Actions View deployment

pan-kot force-pushed the editable-cells-wrapper branch from 0d64af7 to b2a1194 Compare November 28, 2024 08:58

pan-kot deployed to dev-pages November 28, 2024 09:05 — with GitHub Actions View deployment

pan-kot force-pushed the editable-cells-wrapper branch from b2a1194 to a00bad9 Compare November 28, 2024 09:34

pan-kot deployed to dev-pages November 28, 2024 09:41 — with GitHub Actions View deployment

pan-kot force-pushed the editable-cells-wrapper branch 2 times, most recently from a8ea9ae to 82b8f7c Compare November 28, 2024 11:29

pan-kot deployed to dev-pages November 28, 2024 11:36 — with GitHub Actions View deployment

pan-kot force-pushed the editable-cells-wrapper branch from 82b8f7c to 1492307 Compare November 29, 2024 09:00

pan-kot deployed to dev-pages November 29, 2024 09:07 — with GitHub Actions View deployment

pan-kot mentioned this pull request Nov 29, 2024

chore: Updates table cell permutations page #3071

Merged

pan-kot force-pushed the editable-cells-wrapper branch from 1492307 to 6a78971 Compare November 29, 2024 10:53

pan-kot changed the title ~~chore: Editable cells wrapper~~ fix: Fixes table editable cell styles and simplifies it [WIP] Nov 29, 2024

pan-kot deployed to dev-pages November 29, 2024 11:01 — with GitHub Actions View deployment

pan-kot force-pushed the editable-cells-wrapper branch from 6a78971 to 1845521 Compare November 29, 2024 12:54

pan-kot deployed to dev-pages November 29, 2024 13:01 — with GitHub Actions View deployment

body cell wrapper inner

c0e7ae6

pan-kot force-pushed the editable-cells-wrapper branch from 1845521 to c0e7ae6 Compare December 2, 2024 05:52

pan-kot deployed to dev-pages December 2, 2024 05:59 — with GitHub Actions View deployment

pan-kot changed the title ~~fix: Fixes table editable cell styles and simplifies it [WIP]~~ fix: Fixes table editable cell styles and simplifies it Dec 3, 2024

pan-kot commented Dec 3, 2024

View reviewed changes

pan-kot marked this pull request as ready for review December 3, 2024 13:06

pan-kot requested a review from a team as a code owner December 3, 2024 13:06

pan-kot requested review from YueyingLu and removed request for a team December 3, 2024 13:06

Merge branch 'main' into editable-cells-wrapper

da64213

pan-kot deployed to dev-pages December 6, 2024 06:17 — with GitHub Actions View deployment

jperals requested a review from georgylobko December 9, 2024 10:32

georgylobko reviewed Dec 9, 2024

View reviewed changes

fix: Reverted editable cell overflow visible and cell negative vertic…

9836e89

…al margin

pan-kot deployed to dev-pages December 9, 2024 15:23 — with GitHub Actions View deployment

YueyingLu approved these changes Dec 9, 2024

View reviewed changes

georgylobko approved these changes Dec 9, 2024

View reviewed changes

pan-kot added this pull request to the merge queue Dec 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2024

pan-kot added this pull request to the merge queue Dec 16, 2024

Merged via the queue into main with commit 468db9e Dec 16, 2024
39 checks passed

pan-kot deleted the editable-cells-wrapper branch December 16, 2024 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixes table editable cell styles and simplifies it #3061

fix: Fixes table editable cell styles and simplifies it #3061

pan-kot commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

pan-kot Dec 3, 2024

pan-kot Dec 9, 2024

pan-kot Dec 3, 2024

pan-kot Dec 3, 2024

pan-kot Dec 3, 2024

pan-kot Dec 3, 2024 •

edited

Loading

pan-kot Dec 9, 2024

pan-kot Dec 3, 2024

pan-kot Dec 3, 2024

pan-kot Dec 3, 2024

georgylobko Dec 9, 2024

pan-kot Dec 9, 2024

YueyingLu commented Dec 9, 2024

pan-kot commented Dec 9, 2024

fix: Fixes table editable cell styles and simplifies it #3061

fix: Fixes table editable cell styles and simplifies it #3061

Conversation

pan-kot commented Nov 26, 2024 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan-kot Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YueyingLu commented Dec 9, 2024

pan-kot commented Dec 9, 2024

pan-kot commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

pan-kot Dec 3, 2024 •

edited

Loading