-
Notifications
You must be signed in to change notification settings - Fork 44
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
AdvancedTable
: create component (clean branch)
#2615
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
92990e4
to
5409871
Compare
908e670
to
a694ee0
Compare
a694ee0
to
d1c4c2f
Compare
showcase/tests/integration/components/hds/advanced-table/index-test.js
Outdated
Show resolved
Hide resolved
class={{this.classNames}} | ||
...attributes | ||
role="grid" | ||
aria-describedby={{this._captionId}} |
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.
@MelSumner heads up: I ended up using aria-describedby instead of aria-description for the advanced table caption because it needs to be aria-live="polite"
} | ||
} | ||
|
||
if (key === 'Escape') { |
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.
@MelSumner another heads up, after looking into the options for the issue with escape closing both the tooltip and re-focusing the cell - there is no option that scales well here to try and prevent that. It would be very hacky (like making a new tooltip just for advanced table) and wouldn't work if a consumer put something in a cell that also has an escape key handler.
Leaving the behavior as is.
d1c4c2f
to
2034524
Compare
2034524
to
0ecbd25
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.
Thanks for cleaning-up the git history for this work! π
I did a first high-level pass, skimming through the code and checking the component in showcase. Here are my main findings so far:
Issue: keyboard navigation + expandable rows: navigate inside a cell (pressing Enter) and moving focus from the expand button to the next in-cell element (by pressing the right arrow key); at this stage pressing escape will not return focus to the table cell; the user has to return to the expand button and then press escape.
Issue: keyboard navigation + sticky header: navigating upwards (pressing the up key) results in a tricky state where the focused cell is overlapping the header
Issue: keyboard navigation: in-cell navigation works as expected for left/right arrows, but moves the focus outside the cell when up/down arrows are pressed.
packages/components/src/components/hds/advanced-table/helpers.ts
Outdated
Show resolved
Hide resolved
packages/components/src/components/hds/advanced-table/helpers.ts
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/advanced-table/index-test.js
Outdated
Show resolved
Hide resolved
showcase/tests/integration/components/hds/advanced-table/index-test.js
Outdated
Show resolved
Hide resolved
packages/components/src/components/hds/advanced-table/th-button-expand.hbs
Show resolved
Hide resolved
3db2709
to
30fa2fe
Compare
β¦ rounded bottom borders
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.
I found a few things. I believe Andrew will do an independent review too. But here are a few ones. Let me know if you need anything.
π Summary
If merged, this PR would add the AdvancedTable component and related sub-components
π οΈ Detailed description
AdvancedTable showcase
The easiest way to review this would be to step through each commit. I have also left a couple comments/questions inline.
chore: add changeset (easy peasy)
feat: copy Hds::Table and rework to Hds::AdvancedTable
Changes of note:
div
elements with roles instead of table elementsdisplay: contents
which doesn't allow background-colorfeat: copy tests from Table and rework for AdvancedTable
Changes of note:
feat: copy showcase from Table and rework for AdvancedTable
Changes of note:
feat: add keyboard navigation + related tests
Changes of note:
feat: add @hasStickyHeader argument and related showcase example, test
Changes of note:
feat: add nested rows, @childrenKey arg
Changes of note:
chore: skip sticky header test for now
Will improve the sticky header test as a follow up pr.
fix: add cleanup for IntersectionObserver
will-destroy
lifecycle event to clean up the intersection observer.feat: use ember-focus-trap and tabbable libraries
πΈ Screenshots
π External links
Jira ticket: HDS-3965
Figma file: [if it applies]
π Component checklist
π¬ Please consider using conventional comments when reviewing this PR.