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

Jump to bottom

AdvancedTable: follow ups #2659

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

AdvancedTable: follow ups #2659

wants to merge 9 commits into from
+697 −689

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Jan 21, 2025 •
edited
Loading

📌 Summary

  • Remove utility classes from the Td component
  • Prevent default when user presses arrow keys while focused on a cell
  • Move repeated code from td, th, and th-sort to a new hds-advanced-table-cell modifier

📸 Screenshots

🔗 External links

Jira ticket: HDS-4381

👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@vercel Vercel
Copy link

vercel bot commented Jan 21, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Feb 7, 2025 3:10pm
hds-website ✅ Ready (Inspect) Visit Preview Feb 7, 2025 3:10pm

@shleewhite shleewhite changed the title Advanced Table: follow ups AdvancedTable: follow ups Jan 21, 2025
@shleewhite shleewhite force-pushed the hds-4381/advanced-table-follow-up branch from fa9e7f4 to e735248 Compare February 4, 2025 20:58
shleewhite
shleewhite commented Feb 4, 2025
View reviewed changes
packages/components/src/components/hds/advanced-table/td.hbs
{{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
{{focus-trap
isActive=this._shouldTrapFocus
shouldSelfFocus=true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed shouldSelfFocus because it doesn't do anything when initialFocus is set (see within focusTrapOptions)

@shleewhite shleewhite force-pushed the hds-4381/advanced-table-follow-up branch from 936b767 to 45491a7 Compare February 5, 2025 19:38
@shleewhite shleewhite force-pushed the hds-4381/advanced-table-follow-up branch from 5ed9685 to b9e7dd0 Compare February 5, 2025 19:49
@shleewhite
Copy link
Contributor Author

I went back and forth on testing that the is-pinned class gets added correctly on scroll and ultimately got a solution that works most of the time locally, but it still fails in the headless browser in CI. Removing that part of the test so at least theres a test that the one class gets added.

@shleewhite shleewhite marked this pull request as ready for review February 5, 2025 20:23
@shleewhite shleewhite requested a review from a team as a code owner February 5, 2025 20:23
dchyun
dchyun reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@dchyun dchyun left a comment

Choose a reason for hiding this comment

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

Great refactor to a custom modifier, left one small changeset comment.

.changeset/thin-masks-attack.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dchyun dchyun dchyun approved these changes

Assignees
No one assigned
Labels
packages/components showcase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shleewhite @dchyun @hashibot-hds