USE-61: Implement page chrome around the results list #276
Conversation
djanelle-mit
changed the title
djanelle-mit
changed the title
djanelle-mit
requested review from
JPrevost,
jazairi and
matt-bernhardt
and removed request for
jazairi and
matt-bernhardt
| <%= next_url(@enhanced_query) %> | ||
| </div> | ||
| </nav> | ||
| <div id="pagination"> |
There was a problem hiding this comment.
Non-blocking: On wide screens it feels a bit odd to have the pagination not align with the results and instead align with the fill width content that includes the right side callouts. Since the right side callouts are often not-visible at the same time as the pagination it looks uneven when the intent seems to be to justified pagination.
There was a problem hiding this comment.
Just caught up on Slack and you noted there that this is known.
There was a problem hiding this comment.
This irks me, but I think it's a practical starting point for the short term until we have the stepped pagination ("Show X more")
| @@ -0,0 +1,18 @@ | |||
| <div id="callout-wrapper"> | |||
There was a problem hiding this comment.
Non-blocking: This appears to be using 4 spaces instead of two whereas most of our views are using 2 spaces for indents.
There was a problem hiding this comment.
Updated my VS code to default to tabs with 2 spaces. Those were net new files so they used the 4 space default.
| @@ -0,0 +1,18 @@ | |||
| <aside id="results-sidebar"> | |||
There was a problem hiding this comment.
Non-blocking: This appears to be using 4 spaces instead of two whereas most of our views are using 2 spaces for indents.
There was a problem hiding this comment.
Updated my VS code to default to tabs with 2 spaces. Those were net new files so they used the 4 space default.
app/views/search/results.html.erb
Outdated
| <div class="no-results"> | ||
| <p class="hd-2">No results found for your search</p> | ||
| </div> | ||
| </main> |
There was a problem hiding this comment.
This tag ends up closed twice in certain scenarios I believe.
I'd suggest realigning the if/elsif statements so it is easier to keep track of them. I suspect the line 45 closing main might need to move into a condition? Not sure though.
There was a problem hiding this comment.
If it isn't clear what I'm suggesting, I'd be happy to put in a commit and hop on a call to explain.
There was a problem hiding this comment.
I pushed a new version that reorganizes the markup a bit. I checked both results and no results on local and the deployed version and things look good.
Is this what you had in mind? I removed all of the duplicate conditionals and things are easier to read.
I also brought the #results-layout-wrapper into the No Results version to keep the nested styles working as intended. This will help us when it's time to lay out whatever we decide here.
There was a problem hiding this comment.
Yes, all the tags close as expected now (I think!). 👍🏻
Pull Request Test Coverage Report for Build 19438742667Details
💛 - Coveralls |
Developer
This work implements the page chrome UI elements around the results list. This includes:
This work does not include:
Note for reviewer:
This work required some structural changes in the results view. Things appear to be working correctly but double-checking the rails logic in this partial would be wise.
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