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.

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

feat(Table): new sort behavior #1935

Merged
merged 14 commits into from
Mar 6, 2024

Conversation

YossiSaadi
Copy link
Contributor

@YossiSaadi YossiSaadi requested a review from a team as a code owner February 8, 2024 08:26
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Table---New-sort-behavior-5898339907 branch from a3c64e7 to 292b18a Compare February 8, 2024 08:27
@YossiSaadi YossiSaadi marked this pull request as draft February 12, 2024 07:53
@YossiSaadi
Copy link
Contributor Author

Not ready yet - design wants extra UX functionalities

@YossiSaadi YossiSaadi changed the title chore(Table): a11y - set aria-sort to "none" if no sort exists currently for column feat(Table): new sort behavior Feb 14, 2024
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Table---New-sort-behavior-5898339907 branch from fe9ac86 to 0d4ccf7 Compare February 14, 2024 12:32
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Table---New-sort-behavior-5898339907 branch from 0d4ccf7 to 9d5ed1e Compare February 14, 2024 12:34
@YossiSaadi YossiSaadi marked this pull request as ready for review February 27, 2024 13:02
@YossiSaadi YossiSaadi requested review from a team and removed request for a team February 27, 2024 13:02
…39907

# Conflicts:
#	packages/core/src/components/Table/TableHeaderCell/TableHeaderCell.module.scss
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Table---New-sort-behavior-5898339907 branch from 621e496 to 5828ee9 Compare March 6, 2024 07:40
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Amazing!

cleanup();
const { getByText } = renderComponent({ onBlur, blurOnMouseUp: false });
const { getByText } = render(
<Button onBlur={onBlur} blurOnMouseUp={false}>
Copy link
Member

Choose a reason for hiding this comment

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

I didn't even notice that we have this prop

Comment on lines +57 to +58
aria-sort={onSortClicked ? ariaSort : undefined}
tabIndex={onSortClicked ? 0 : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

null instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on purpose, since I do not want the prop to be defined, it is the same as we're not passing a prop at all
for aria-sort for example, I do not want to show aria-sort in the DOM since a screen-reader can think it is sortable
for tabIndex, null is not a correct value, and I want to use the default

@YossiSaadi YossiSaadi force-pushed the feat/yossi/Table---New-sort-behavior-5898339907 branch from bc04a4a to fc1b36b Compare March 6, 2024 11:28
@YossiSaadi YossiSaadi merged commit 32790f4 into master Mar 6, 2024
8 of 9 checks passed
@YossiSaadi YossiSaadi deleted the feat/yossi/Table---New-sort-behavior-5898339907 branch March 6, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants