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

[EuiTablePagination] Add aria-current to rows per page menu #7186

Merged
merged 9 commits into from
Sep 14, 2023
Merged

[EuiTablePagination] Add aria-current to rows per page menu #7186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1bc5129
Added checks for aria-current to table pagination button and context …
1Copenut Sep 11, 2023
aed046a
Merge branch 'main' into feature/euiPagination-aria-current
1Copenut Sep 13, 2023
f8b416f
Refactoring current item checks into a single function.
1Copenut Sep 13, 2023
52a9c42
Updating snapshot and adding CHANGELOG.
1Copenut Sep 13, 2023
fdf70d9
Adding setActiveProps to useMemo dep array.
1Copenut Sep 13, 2023
5afd8d5
Removing itemsPerPage from useMemo dep array.
1Copenut Sep 13, 2023
42226b0
Update upcoming_changelogs/7186.md
1Copenut Sep 14, 2023
d962054
Refactoring aria-current and icon to simpler ternary from util func.
1Copenut Sep 14, 2023
0a1e8c6
Moving key to first prop declaration for clarity.
1Copenut Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/table/table_pagination/__snapshots__/table_pagination.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ exports[`EuiTablePagination renders 1`] = `
>
<div>
<button
aria-current="true"
class="euiContextMenuItem"
data-test-subj="tablePagination-10-rows"
type="button"
Expand Down
3 changes: 3 additions & 0 deletions src/components/table/table_pagination/table_pagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ export const EuiTablePagination: FunctionComponent<EuiTablePaginationProps> = (
<EuiContextMenuItem
key={itemsPerPageOption}
Copy link
Member

@cee-chen cee-chen Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This key is still needed for .map iteration. You should be see a ton of warnings in the console without it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I re-added the icon and aria-current before the key, so the default PR view shows it removed. The key is still in the code on L#121. I can move it back to previous location.

Copy link
Member

Choose a reason for hiding this comment

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

oh whoops, you're right 🤦 yeah let's make the key the first prop as a general rule, it makes it easier to tell it's there since it's a "special" prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that as an EUI house style and sound argument. I'll move it to the first prop declaration and set auto-merge after the tests run.

icon={itemsPerPageOption === itemsPerPage ? 'check' : 'empty'}
aria-current={
itemsPerPageOption === itemsPerPage ? 'true' : undefined
}
onClick={() => {
closePopover();
onChangeItemsPerPage?.(itemsPerPageOption);
Expand Down
3 changes: 3 additions & 0 deletions upcoming_changelogs/7186.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Accessibility**

- Added `aria-current` attribute to `EuiTablePagination`
Loading