For reference only #272
Merged
For reference only #272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
** Why are these changes being introduced: We want to add an `aria-current="page"` attribute to the currently active tab link on a results page, so that assistive technologies can parse which is the active tab. This requires a conditional link_to call, as well as an update to the javascript which handles tab changes. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-180 ** How does this address that need: Because we can't (apparently) write a conditional inside an argument to a link_to call, we have to maintain the conditional check elsewhere - so to prevent the source_tabs partial from becoming a mess of multiple conditionas, we define a new search helper function, link_to_tab. This takes a string as an argument, and is capable of outputting both a link to the active tab, as well as to an inactive tab. We also update loading_spinner.js to be able to maintain the state of the aria-current attribute. ** Document any side effects to this change: The tests for this helper function _should_ be robust enough, but I worry that they're a bit too clever. I didn't want to just write a string comparison assertion, because that feels very fragile given how much this partial has been changing lately. The assertions as written should focus on the values being present or not, but still be robust to other changes. Also note that, for now, the source_tabs partial is still a bit bespoke in that there are explicitly two calls to the various tabs. I'd originally intended to define a list in the controller, but managing the list of tabs in this way feels awkward and like it doesn't gain anything since the tabs are entwined with so much of the rest of the application. Abstracting the tab order and contents to a list doesn't save us from other complexity.
This moves the manipulation of the tabs to the click action (via a standalone function) rather than in the turbo:frame-render action.
Pull Request Test Coverage Report for Build 19308575149Details
💛 - Coveralls |
Member
Author
|
I feel the need to clarify something here for future-us. This branch wasn't merged, but an alternative PR that extended this one was. The actual changeset that merged (which included these commits, and also two additional ones) was #271 . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Developer
Accessibility
New ENV
Approval beyond code review
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
added technical debt.
Documentation
(not just this pull request message).
Testing