Skip to content

Conversation

@matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Nov 18, 2025

We recently refactored how the spinner is applied and removed when changing between tabs, but neglected to be consistent when it comes to pagination changes.

Relevant ticket:

https://mitlibraries.atlassian.net/browse/use-201

How does this address that need:

This adds the same add/remove calls for managing the spinner to the pagination management branches of the javascript file. It elects to maintain the separate pathways for these calls, rather than trying to handle the spinner in a unified area of the event listeners.

Side effects:

The duplication of calls, like lines 46-50 being basically the same as lines 26-30, may be seen as a code smell later. For now, though, I prefer to keep them separate like this - both for parity between these pathways and also symmetry with the .classList.add() lines at 61 and 75.

It seems safer to handle the spinner only in qualified contexts, rather than risking it appearing or disappearing without that being intended. I suspect that what's needed is an ARIA live region around the results, with maybe a status

Developer

Accessibility

While I've run WAVE, I'm not sure this is the right tool for a PR like this - but I don't know of another one. I've confirmed via VoiceOver that the focus moves back to the top of the results list, and that the spinner doesn't interfere with the user's ability to move through the new results.

What I'm not certain about is what the best way is to advise the user that the focus has changed. I suspect that what might be needed would be to wrap the spinner with an aria-role="status" attribute, while also declaring the results list to be a live region - but this feels non-trivial and something I'd like to dig into specifically, rather than handle it incidentally while fixing how pagination gets the spinner or not.

I'll figure out the right venue to raise this for the project team.

New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively.
  • UXWS/stakeholder review is not needed.
Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

** Why are these changes being introduced:

We recently refactored how the spinner is applied and removed when
changing between tabs, but neglected to be consistent when it comes to
pagination changes.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/use-201

** How does this address that need:

This adds the same add/remove calls for managing the spinner to the
pagination management branches of the javascript file. It elects to
maintain the separate pathways for these calls, rather than trying to
handle the spinner in a unified area of the event listeners.

** Document any side effects to this change:

The duplication of calls, like lines 46-50 being basically the same as
lines 26-30, may be seen as a code smell later. For now, though, I
prefer to keep them separate like this - both for parity between these
pathways and also symmetry with the .classList.add() lines at 61 and 75.

It seems safer to handle the spinner only in qualified contexts, rather
than risking it appearing or disappearing without that being intended.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 19482155291

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.992%

Totals Coverage Status
Change from base Build 19478505178: 0.0%
Covered Lines: 927
Relevant Lines: 946

💛 - Coveralls

@mitlib mitlib temporarily deployed to timdex-ui-pi-use-201-sp-rvhvks November 18, 2025 22:17 Inactive
@JPrevost JPrevost self-assigned this Nov 19, 2025
@matt-bernhardt matt-bernhardt merged commit 3bfa107 into main Nov 19, 2025
5 checks passed
@matt-bernhardt matt-bernhardt deleted the use-201-spinner branch November 19, 2025 19:25
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.

5 participants