Use 65 tacos interventions #241
Conversation
Pull Request Test Coverage Report for Build 18603852278Details
💛 - Coveralls |
|
From a front-end perspective, I think style-wise we're pretty safe to ship here. Functionally, I don't think we need the icon here. It makes these too similar to our Alert pattern, and creates an awkward visual space on the left. Let's remove it and have the content be the leftmost thing. I'll create a ticket to follow up on some small style tweaks that will be dependent on the markup and style of the sections above the intervention and pick it up when the time is right. |
There was a problem hiding this comment.
This feels straightforward enough, thanks for keeping the removal and addition cleanly separated by commit.
I don't think I have any blocking concerns - one question to make sure I understand the long term strategy for stylesheets, and one a11y-related question about where in the DOM the interventions belong - but those are also both things I can think can be resolved in future tickets.
Of these, I'm most concerned about the a11y issue, but in full disclosure I think there's at least one other a11y-related bug in the results page - or at least a problem I encountered listening to results in voiceover just now. The reader gets stuck after every search result down the list, and seems to jump backwards to the previous "full record" button and just stop there. I'm not sure whether this is a problem with my setup, or an issue with the markup we have - but I'm choosing for now to lean on our future a11y testing to give us space to consider issues like this.
I'm happy to make a ticket for the a11y issue if you'd like me to do so. Finding the right location in the DOM for the intervention feels like it might be better handled under the general UI redesign? GeoData doesn't have these interventions, I think, and that's the only one of these applications that's currently in production.
| @@ -0,0 +1,106 @@ | |||
| /* Color Variables */ | |||
There was a problem hiding this comment.
Non-blocking question (and maybe more for @djanelle-mit than you) - reading this stylesheet, it feels like the long-term plan for many of these variables, mixins, and other styles is to house them more centrally to the application stylesheet setup? Ultimately I think this partial could end up being just lines 27-59?
I don't mind this change as-is for now, but I want to confirm whether I understand the long-term plan correctly.
There was a problem hiding this comment.
@matt-bernhardt Yep, you're spot on! This was originally written into the TACOS repo with the variables colocated to keep things simple. I'm also going to be introducing more variables into the _variables style sheet with some of my upcoming work.
I have a ticket to update some styling, I can make a note that it should include a refactor of the variables' location, too.
app/views/search/results.html.erb
Outdated
| <%= render(partial: 'search/doi') if fact_enabled?('doi') %> | ||
| </div> | ||
| <% end %> | ||
| <%= render(partial: 'trigger_tacos') if tacos_enabled? %> |
There was a problem hiding this comment.
This may be something to deal with later, I'm not sure - but this location in the DOM moves the interventions panel before the target of the "skip to main content" link - so a user navigating with their keyboard who uses this link will not end up hearing about the intervention.
There was a problem hiding this comment.
I'll take a look at this, thanks
There was a problem hiding this comment.
This is fairly complicated to skip to before this content when it may not be there. Skip links need to target a focusable area and the closest one before this as it currently exists is the form.
I've asked UX for clarification as to whether this is even the correct location because if they were envisioning this inside a tab then it gets much easier as we can just focus the tab. If they wanted this to insert where it is, we'll need to consider our options (one of which is to deal with it later of course, but that one doesn't feel great).
There was a problem hiding this comment.
@matt-bernhardt can you look at the change I just introduced. It moves the intervention below the tabs (but maybe not in a way that will work once they look like tabs... but that's problem for later... and updated the skip link to target the tabs. It's still weird in Safari, but I think it's been like that long enough to not try to wrap up a solution in this PR. I linked you a ticket and started a thread in Slack to explore what you were seeing in relation to issues we've had backlogged for a while now.
There was a problem hiding this comment.
I think this looks good. I think I see the Safari weirdness you reference (around skip-link behavior and focus), but as you note that's not caused by this change (I've confirmed I see it in our other apps already).
I'm happy to formally re-review and approve this, but it looks like my last review still works for that if you're willing to use that. Thanks for opening the conversation with UX about where in the DOM this panel needs to go for screen readers to catch it.
| tacos_response = Tacos.analyze(params[:q]) | ||
|
|
||
| # Suggestions return as an array but we don't want to display more than one. | ||
| # We may want to have a "priority" system in the future to determine which suggestion to show. |
There was a problem hiding this comment.
+1 to having this sort of logic, and +1 to not worrying about it now. The time for that sort of determination feels like "after we are getting more than just suggested resources back from TACOS" because then we'll have more things to sort through, in addition to multiple suggested resource matches.
JPrevost
force-pushed
the
use-65-tacos-interventions
branch
from
dcfa91a to
9fe969e
Compare
Why are these changes being introduced: * We are moving to TACOS suggestions instead of fact panels * Fact panels was an initial implementation we later extracted into TACOS Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-65 Document any side effects to this change: * Not all fact panels will be replaced initially with TACOS suggestions
Why are these changes being introduced: * Displaying panels with TACOS suggestions on the search results and pages when they are available will help users access the resources they need more quickly. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-65 How does this address that need: * Brings in the draft TACOS styles and views that Dave build in TACOS and modifies them to fit into the existing application styles and views. * Extracts the suggestions from the tacos response and displays them in a panel on the search results and analyze pages. Document any side effects to this change: * Only added to non-GDT searches at this time. Co-Authored-By: djanelle-mit <181018533+djanelle-mit@users.noreply.github.com>
JPrevost
force-pushed
the
use-65-tacos-interventions
branch
from
9fe969e to
0ba8da8
Compare
5 participants
NOTE: You can see examples of the panel in the PR build by searching for "hours" or "iso 9001".
NOTE: It may be easier to review the two commits separately as one removes our legacy Fact panels and the other adds the new Suggested panel.
Why are these changes being introduced:
and pages when they are available will help users access the resources
they need more quickly.
Relevant ticket(s):
How does this address that need:
and modifies them to fit into the existing application styles and
views.
in a panel on the search results and analyze pages.
Document any side effects to this change:
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