feat(table): s2 table migration layout part-1 #3799
Conversation
marissahuysentruyt
added
size-5 L
|
🚀 Deployed on https://pr-3799--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.42 MB*
table
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-1178-s2-table-migration-layout-pt1
branch
from
25578c0 to
8a6fb2f
Compare
🦋 Changeset detectedLatest commit: 101b69b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-1178-s2-table-migration-layout-pt1
branch
2 times, most recently
from
36920dd to
490a4e9
Compare
| Visuals.storyName = "With visuals"; | ||
|
|
||
|
|
||
| // TODO: The design team doesn't have support for collapsible rows in the table component, so they are removed from the docs page for now. |
There was a problem hiding this comment.
I have a couple TODO notes throughout the stories file here, reflecting on the conversation I had with Stefan.
Design doesn't have collapsible row designs, or dropzone designs done (and they're unsure if they will). React only has beta drag & drop table components.
So I didn't really want to get rid of our stories or tests for those table variants/options. What are our thoughts on just removing these 5 or 6 stories regarding collapsible rows and dropzones from the autodocs?
| Sizing.args = {}; | ||
| Sizing.tags = ["!dev"]; | ||
| Sizing.parameters = { | ||
| export const EmptyState = Template.bind({}); |
There was a problem hiding this comment.
I'd love some feedback on the empty and loading states. Should I do anything more than return the illustrated message or progress circle component? As in, do I return the header row, AND THEN render the illustrated message/progress circle, with those 2 components like "in" the table? That's not really what Figma looks like, but...open to feedback on these two new stories.
There was a problem hiding this comment.
I would not render any of the table semantics. We could talk to design about having a "skeleton" loading state to populate a few rows which would be wrapped with aria-busy="true" until the data loaded, as described by Sara. That would also have a benefit of holding space and possibly reducing a page jump when content came in. It might be appropriate to couple with a live region as well, which would be more app-dependent.
There was a problem hiding this comment.
Oh I love the idea of a skeleton loader. However, that also sounds like a separate ticket and PR. I'll make a ticket for it, along with some of the comments above, just so we have this idea documented! Thank you!
| > | ||
| ${when(showCheckbox && !isSectionHeader, () => html` | ||
| <${cellTag} | ||
| role="gridcell" | ||
| class=${classMap({ | ||
| [`${rootClass}-cell`]: true, | ||
| [`${rootClass}-checkboxCell`]: true, | ||
| [`${rootClass}-cell--alignEnd`]: getTextAlignment(0) === "end", | ||
| ...cellCustomClasses?.[0]?.reduce((a, c) => ({ ...a, [c]: true }), {}), |
There was a problem hiding this comment.
I wanted to show the individual cell focus states, so I had to pass custom classes to the cells.
components/table/stories/template.js
Outdated
| })} | ||
| tabindex="0" |
There was a problem hiding this comment.
The rows should be keyboard focusable, as well as the cells (according to the designs), so I added the tabindex in quite a few places.
Any accessibility concerns with how everything is laid out right now? I don't know the flow for tables. Should each focused row move to the next focusable row, or would a user focus on a row, then move through the cells?
There was a problem hiding this comment.
The tabindex should not be included by default, as screen readers have a specific mode to navigate tables. And for keyboard nav, it may be ok to use the roving tabindex technique like React does, but that is JS-controlled based on key presses.
A tab stop tends to indicate some kind of interaction, so if the row isn't selectable, it shouldn't be a tab stop. And if it is selectable, then again I think roving tabindex is more appropriate than forcing every row to be a tab stop via tabindex. But - implementing roving tabindex would be on SWC, I think.
It looks like SWC currently doesn't support sortable columns, but if we are demonstrating that, then it still wouldn't be the th receiving focus, but a nested button like this accessible sortable table example
An exception is the scroller utility, which should have tabindex="0" to allow keyboard access to scroll that region, which is also demonstrated in the sortable table article.
There was a problem hiding this comment.
Thank you for all of this info and the example! I removed the tab index instances I added ✅ (except for one- the third column header cell was the only header cell that didn't have the tabindex).
As for the scroller utility- we demo the scrollable , and I can scroll with the arrow keys, no problem, both with table elements and made entirely of divs.
I may add some documentation to the "Row states" story (that has a bunch of different focus and selected combinations) to mention something about "implementations should add appropriate tab stops to indicate focus is on a row."
There was a problem hiding this comment.
Ok, I think that tabindex will go away when you update this to be a button instead (per Slack discussion), since that will provide the tab stop.
Scrolling via keyboard works as I expect 👍
| */ | ||
| export const Sizing = (args, context) => Sizes({ |
There was a problem hiding this comment.
Doesn't look like we ever really supported sizes actually. The guidance site doesn't refer to table sizes, nor do the current Figma designs for S2 🤷♀️
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-1178-s2-table-migration-layout-pt1
branch
from
14fcf31 to
4b76b0c
Compare
| @@ -204,65 +285,75 @@ export const Template = ({ | |||
| id=${ifDefined(id)} | |||
| role=${ifDefined(useCheckboxCell ? "grid" : useDivs ? "table" : undefined)} | |||
| aria-multiselectable=${ifDefined(useCheckboxCell ? "true" : undefined)} | |||
| style="max-width: 800px;" | |||
There was a problem hiding this comment.
Should I keep this? Is it necessary? I'm fine with adding this max-width back on, just let me know!
| isChecked: false, | ||
| isIndeterminate: true, | ||
| customClasses: [`${rootClass}-checkbox`], | ||
| ${when(selectionMode === "multiple", () => html` |
There was a problem hiding this comment.
Checkboxes should only appear in the thead if users can select more than one row (and this puts the indeterminate checkbox in that header row). Otherwise, checkboxes for single select rows just appear in their rows.
components/table/stories/template.js
Outdated
| tabindex="0" | ||
| > | ||
| ${Icon({ | ||
| iconName: "Sort", |
There was a problem hiding this comment.
I added each of the new sort icons to each header cell.
There was a problem hiding this comment.
I wonder if we should remove these as far as the default display, and only have them within a Story related to sorting? Then we can also include any relevant docs for using sorting.
There was a problem hiding this comment.
Sure I can do that!
There was a problem hiding this comment.
New story has good info! Will re-look when updated to the button.
components/table/stories/template.js
Outdated
| customClasses: [`${rootClass}-sortIcon`], | ||
| }, context)} | ||
| <span class="${rootClass}-columnTitle">Column title</span> | ||
| ${Icon({ |
There was a problem hiding this comment.
The second column now has the additional "menu" or disclosure icon. It's non functional but...should this be a button? Like an action button to trigger a menu?
The rest of the collapsible disclosure icons are all buttons so...that's what triggered the question for me.
There was a problem hiding this comment.
It's possible both this and the sortable icon (noted in the "tabindex" comment) should be a button.
Maybe first get ideas from design on what is going to show up in the menu? For example, if it's just going to show the sort options, then that would result in one button that launches the menu vs. separate actions.
The scenario of two, separate actions is a little fuzzy to me as far as handling the labeling and sorted status accessibly... maybe if design validates that could happen, you could describe the scenario and ask in the a11y Slack channel?
There was a problem hiding this comment.
I asked Stefan about the possibilities for the menu items. 👍 I'll also send you a React link in slack for some of their menu options.
There was a problem hiding this comment.
Is this thread related to the disclosure icon that's in the designs? I've not been able to find a story where this appears.
components/table/index.css
Outdated
| &::after { | ||
| content: ""; | ||
| position: absolute; | ||
| inset: calc(-1 * var(--mod-table-border-width, var(--spectrum-table-border-width))); | ||
| border-radius: calc(var(--mod-table-border-radius, var(--spectrum-table-border-radius)) - var(--mod-table-border-width, var(--spectrum-table-border-width))); | ||
| border: var(--mod-table-focus-indicator-thickness, var(--spectrum-table-focus-indicator-thickness)) solid var(--highcontrast-table-cell-focus-indicator-color, var(--highcontrast-table-focus-indicator-color, var(--mod-table-focus-indicator-color, var(--spectrum-table-focus-indicator-color)))); | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
The cells have a different focus indicator border radius, than they do when non-focused. I'd love some feedback on this ::after approach to achieve that.
| --highcontrast-table-icon-color: var(--highcontrast-table-row-text-color-hover); | ||
| --spectrum-table-cell-background-color: var(--highcontrast-table-row-background-color-hover, var(--mod-table-row-active-color, var(--spectrum-table-row-active-color))); | ||
| } | ||
| /* First cell of focused rows need fancy new focus indicator borders & no border-radius, and the thicker row focus indicator */ |
There was a problem hiding this comment.
This was pretty complex, so again- feedback is welcome!
The table row itself can be focused.
It's got a 4px blue side focus indicator, as well as (basically) a border that goes around the entire row.
Some rows don't have border radii, and some do. But- those border radii are created with the individual table cells in that row.
🥵🥵🥵
There was a problem hiding this comment.
I noticed there is some content shift happening when focusing on a row (which I didn't notice before). Anybody have ideas on how to avoid that? I think it's because I am adding the border-block-end to some cells of the row, which ultimately adds an extra 1px.
There was a problem hiding this comment.
I sent a message to Stefan to clarify the "borders around a focused row." That might just be for an emphasized table? I need some clarity on that.
There was a problem hiding this comment.
Looks like you added a different TODO comment re: the shift?
There was a problem hiding this comment.
yes! the layout shift was addressed in this commit: c7a3a1e
| padding-block-start: calc(var(--mod-table-row-top-to-text, var(--spectrum-table-row-top-to-text)) - var(--mod-table-border-width, var(--spectrum-table-border-width))); | ||
| padding-block-end: var(--mod-table-row-bottom-to-text, var(--spectrum-table-row-bottom-to-text)); | ||
| } | ||
| align-items: center; |
There was a problem hiding this comment.
Calling all feedback again!
The guidance says: "All content should be centered vertically within the row to offer the right visual balance." That's why I added the align-items: center, ripped out all of the custom padding, and centered everything everywhere else. That does however, prove difficult in collapsible rows for a number of reasons:
- the button component (with the disclosure icon) is just tall, so the padding around the cells beefs the height up.
- it's really weird to have a cell with wrapped text where everything is aligned to the center
However....collapsible rows aren't quite designed out so we don't have any true guidance on how to handle any of that spacing. I removed them from the docs page (pending others' agreement/approval). Should I reimplement some of this padding and alignment handling since the collapsible table variants are still in the test grid? How would we like to handle this?
There was a problem hiding this comment.
While text that's truly that long is probably an outlier, I could see an argument for start-aligning the icon with the text specifically as an accessibility benefit for users of zoom or screen magnification who see less of the screen at one time. So, having the collapse icon so separated from the labeling text might create a barrier in noticing the visual cue of the icon. (same issue possible for selectable rows)
So - maybe we can bring up this point with them and hold on further changes as far as this PR is concerned?
There was a problem hiding this comment.
Ok, I'll hold on changing anymore for the collapsible variants.
For further deets on this from my conversation with Stefan- he mentioned that really, if design were to support collapsible rows (they don't currently), he would use "accordion items." I'm not sure if he means that engineering should also use an accordion item but that feels semi-related to our conversation about column headers being buttons too.
I'm happy to create another card to revisit this!
There was a problem hiding this comment.
Regardless of the collapsible variant, if "all content should be centered vertically within the row," why do we have top and bottom spacing tokens for rows? 🤔
There was a problem hiding this comment.
According to Stefan, the guidance is out of date. He also mentioned that the "uneven" padding is temporary until the new font is incorporated.
marissahuysentruyt
added
ready-for-review
and removed
wip
-instead, continue the use of the individual cell border-block-start in the quiet variant, but use the divider color instead of border-color
- updates a missing token for focused cells - removes unused token for row focus indicator outline width
- adds story for a table with a menu action button in the header cell - adds hasMenu story arg and new markup
- sets the hasMenu arg to false - adds a better description for the emphasized arg - adds argTypes to the EmptyState story - fixes the action button link - changes "left-aligned" to "start-aligned" - removes section header quiet from side nav
- removes action button markup in favor of regular button - adjusts template for sort-only and menu buttons - updates some class names - sort-only renders a button in the head cell, and menu renders a button in the head cell with the sort icon and disclosure icon
- the sort-only and menu button styles are now the same since the markup changed - new passthrough mods for the button component were refactored from the action button component - removes unused sortIcon and menuIcon selectors/styles
- remove trailingIcon arg - correctly adds aria-multiselectable to the table when the selectionMode is set to "multiple" - updates to docs page
- updates to documentation to clear up additional classes for the sort button and menu button - updates the template to use the new classes - updates the aria sort attribute to be set on the head cell if hasMenu is true - updates the new classes in the CSS
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-1178-s2-table-migration-layout-pt1
branch
from
5f0b680 to
101b69b
Compare
marissahuysentruyt
deleted the
marissahuysentruyt/css-1178-s2-table-migration-layout-pt1
branch
Description
This PR is part 1 of the table's migration to S2! Table has been updated to include:
Layouthave been addressed)!autodocs)NOTE: Tokens for color and typography, variant support for sparklines, and any remaining touch ups (like selected hover/down states) will be done in part 2 of the table migration here: #3818
All styles for the full migration will not be addressed in this PR. Layout changes only!
Jira/Specs
CSS-1178
Figma token specs- layout token column only
CSS-1180- this is a corresponding spike ticket. I used the answers to the questions in that ticket to inform some of my decisions around not exposing certain stories.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
Please review the following table layout-specific styles to ensure the tokens are used as outlined in the tokens spec Figma file:
(@5t3ph)
Additional validation
(@5t3ph)
Regression testing
Validate:
Screenshots
To-do list