Adds persistence to Tab selection across searches #265
Conversation
Pull Request Test Coverage Report for Build 19171924982Details
💛 - Coveralls |
JPrevost
force-pushed
the
use-135-sticky-tabs
branch
from
f6e2169 to
fa50823
Compare
| } | ||
|
|
||
| /* temp style to visualize active tab. Save us from this Dave! */ | ||
| #tabs .active{ |
There was a problem hiding this comment.
@djanelle-mit just tagging you so you know where to remove my amazing code when you pull the tab work soon!
There was a problem hiding this comment.
thanks for the tag and the comment in the css, too!
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
This is working well locally for me. Bonus points for cleaning up a gdt instance. 🙂
In terms of how this affects #264, full page reloads will mean no spinner on tab changes. However, I think that's perfectly fine. We still get the loading bar that Turbo provides for free, which is consistent with the loading behavior for an initial search (also a full page reload).
Please make sure to notify UXWS of this during QA. I spoke with Darcy this morning about loading indicators, and she currently expects a spinner over the results list for both pagination and tab changes. This PR means we will instead get the loading bar for tab changes, with the spinner for pagination changes.
I should note that if UXWS does want the spinner for tab changes, that may still be possible -- I just want to make sure they are aware that this impacts that.
Why are these changes being introduced: * Running multiple searches always reset to the Primo tab. It is expected that isn't what users want. They want to stay on the tab they selected. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-135 * https://mitlibraries.atlassian.net/browse/USE-156 How does this address that need: * Updates the tab links to set a cookie with the selected tab when clicked. * Updates the search form to include the active tab as a hidden field so that when a new search is performed, the selected tab is preserved. * Updates the Tabs Turbo Frame to target the whole page to allow the form element to be updated * This also fixed the issue with back links not updating the form keyword input correctly which was a separately reported bug. Document any side effects to this change: * Refactor to extract some logic from the SearchController to ApplicationController to allow reuse in BasicSearchController. * Minor refactor to conditional logic to always use case and eliminate the if statement that was used only for GeoData. Also normalized to `geodata` language (was `gdt`) to match other wording used in the app.
JPrevost
force-pushed
the
use-135-sticky-tabs
branch
from
fa50823 to
ed6ef11
Compare
Why are these changes being introduced: * Initial implementation used full page refresh but that removed the desired spinners Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-135 How does this address that need: * Moves back to target content refresh with turbo frame * Adds JavaScript to update hidden form element and set active class on tab * Introduces Feature to slow down fast requests when testing page elements that are fleeting like spinners/loaders
jazairi
left a comment
jazairi
left a comment
There was a problem hiding this comment.
Excellent refactor. The new JS is minimal and straightforward, and the latency feature is a nice addition. I'm still seeing the cookie get set as expected. Glad this was relatively easy to implement!
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
geodatalanguage (wasgdt) to match other wording used in the app.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