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

Update pagination style and kb navigation for editions table #9883

Conversation

schu96
Copy link
Contributor

@schu96 schu96 commented Sep 17, 2024

Addresses #9853 tasks 1 and 3

Refactor

Technical

Adds a background to pagination button to make it easier for users to see which page they are currently viewing.

Prevents users from tabbing into the a element within the editions table header row, and adds an outline for the currently focused element within the row. Arrow keys left and right can be also used to switch between the two header rows. No longer implemented since this behavior does not match functionality for similar websites (i.e. Wikipedia)

Pressing enter within a section of the table header row will now update the arrow indicator for ascending/descending order.

Testing

Go to a works page and scroll down to the editions table.

Verify that the current paginated button has a grey background with black text.

Tab into the editions table header row and use Enter/ArrowLeft/ArrowRight/Tab/Shift + Tab to test added keyboard navigation.

Screenshot

2024-09-17.08-21-20.mp4

Stakeholders

@cdrini

@schu96 schu96 force-pushed the 9853/design/editions-table-pagination-kb-navigation-update branch from 4f909c2 to ef63d42 Compare September 17, 2024 15:39
@schu96 schu96 force-pushed the 9853/design/editions-table-pagination-kb-navigation-update branch from ef63d42 to 40c16b0 Compare September 17, 2024 15:51
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 17.05%. Comparing base (ce16a79) to head (ea74c87).
Report is 483 commits behind head on master.

Files with missing lines Patch % Lines
...ary/plugins/openlibrary/js/editions-table/index.js 0.00% 9 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9883      +/-   ##
==========================================
+ Coverage   16.06%   17.05%   +0.99%     
==========================================
  Files          90       89       -1     
  Lines        4769     4725      -44     
  Branches      832      829       -3     
==========================================
+ Hits          766      806      +40     
+ Misses       3480     3409      -71     
+ Partials      523      510      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cdrini cdrini self-assigned this Sep 23, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice this looking very slick! One small styling fix, and a small refactor to DRY up the toggle logic 😊

openlibrary/plugins/openlibrary/js/editions-table/index.js Outdated Show resolved Hide resolved
static/css/components/editions.less Outdated Show resolved Hide resolved
@cdrini
Copy link
Collaborator

cdrini commented Sep 23, 2024

Oh and you'll need to bump the value in the bundlesize.config.js to fix the failing JS test ; change it to 28kb https://github.com/internetarchive/openlibrary/actions/runs/11000579077/job/30543277286?pr=9883#step:9:242

@schu96
Copy link
Contributor Author

schu96 commented Sep 23, 2024

@cdrini Unfortunately I've run into another snag with the page-admin.css bundle size by .01kb :/
Should I also bump up the value for that by .05/.1kb?

@cdrini
Copy link
Collaborator

cdrini commented Sep 24, 2024

@schu96 Ah yes we generally bump up to the next kilobyte to avoid this happening super often 😁

@schu96 schu96 force-pushed the 9853/design/editions-table-pagination-kb-navigation-update branch from bdf278b to 3e68a96 Compare September 24, 2024 17:52
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Lgtm, works like a charm!

@cdrini cdrini merged commit ea9462d into internetarchive:master Sep 24, 2024
4 checks passed
DanielleInkster pushed a commit to DanielleInkster/openlibrary that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants