Fix styling of file attachment navigation and backgrounds.
Review Request #13911 — Created May 30, 2024 and submitted
The Previous/Next file navigation buttons had a few style issues on dark
mode, some regressions, and bad interactions.They were bunched up against the review UI, which regressed a while back
when we renamed thereview-request
CSS class torb-c-review-request
.
That had intended to ensure enough space between the button and the
review UI, but that had broken. That spacing has been restored.The colors used on the navigation buttons have been updated to work in
both light and dark modes.Thumbnail backgrounds now use a darker color by default, and a matching
text color. This gives us text-based file attachment thumbnails that
look appropriate on dark mode. For consistency with rendering, we use
the "diff equals" theme color. We may want to revisit that down the road
when we build a proper CSS component for thumbnails, instead picking the
theme color for text-based attachments only.And finally, an interaction issue with navigation has been fixed. We
were wrapping the entire attachment in an<a href>
, but since that's
an inherently inline element and we're putting complex block-level
elements inside of it, the browsers are forced to try to compensate in
some form. They seem to do that these days by wrapping most inner
elements with their own<a href>
, creating a mess of styles. The end
result doesn't even make for a fully-clickable attachment.The solution is the same as what we use for file attachment thumbnails
on a review request. We now have an<a href>
overlay that sits on top
of the whole element and consumes all click events, giving us the
ability to navigate but also to respect browser link interactions. The
result is a wider clickable area for navigation. This has also been made
accessible.
Tested previous and next navigation in both light and dark mode, with
different types of file attachments.Tested the main review request thumbnails for image-based, PDF-based,
and text-based file attachments.
Summary | ID |
---|---|
2f80ff333b583534a5e906f317e87fa65ca16582 |
Description | From | Last Updated |
---|---|---|
Instead of getting rid of <a> and its browser features, can we give it the file-thumbnail-overlay class, and instead of … |
maubin | |
When looking at the html in Chrome dev tools, I see that this appears as <div class="file-thumbnail"> <div class="file-thumbnail">... </div> … |
maubin |
-
-
Instead of getting rid of
<a>
and its browser features, can we give it thefile-thumbnail-overlay
class, and instead of having all of the content wrapped in the<a>
, make the content a sibling of it. So:<div class="review-ui-prev-attachment"> <div class="file-container"> <div class="file"> <div class="file-thumbnail-container"> <a href="..." class="file-thumbnail-overlay"></a> <div class="file-thumbnail">...</div> </div> <div class="file-caption-container"> ...
I notice this is what we do for file attachment thumbnails on the review request page. I do like to use
Ctrl+Click
when navigating, so I wouldn't like to see it go. -
When looking at the html in Chrome dev tools, I see that this appears as
<div class="file-thumbnail"> <div class="file-thumbnail">... </div> <div>
So looks like the
file-thumbnail
div is included in the{{prev_file_attachment.thumbnail}}
. Can we remove the outer div here?
- Change Summary:
-
- Went back to
<a href>
-based click management using an overlay approach. - Removed a redundant
file-thumbnail
. - Added accessibility attributes.
- Went back to
- Description:
-
The Previous/Next file navigation buttons had a few style issues on dark
mode, some regressions, and bad interactions. They were bunched up against the review UI, which regressed a while back
when we renamed the review-request
CSS class torb-c-review-request
.That had intended to ensure enough space between the button and the review UI, but that had broken. That spacing has been restored. The colors used on the navigation buttons have been updated to work in
both light and dark modes. Thumbnail backgrounds now use a darker color by default, and a matching
text color. This gives us text-based file attachment thumbnails that look appropriate on dark mode. For consistency with rendering, we use the "diff equals" theme color. We may want to revisit that down the road when we build a proper CSS component for thumbnails, instead picking the theme color for text-based attachments only. And finally, an interaction issue with navigation has been fixed. We
were wrapping the entire attachment in an <a href>
, but since that'san inherently inline element and we're putting complex block-level elements inside of it, the browsers are forced to try to compensate in some form. They seem to do that these days by wrapping most inner elements with their own <a href>
, creating a mess of styles. The endresult doesn't even make for a fully-clickable attachment. ~ We now use
onclick
withRB.navigateTo
instead. This is effectively~ equivalent to an <a href>
(though without browser features for opening~ in a new tab -- certainly a regression, but perhaps necessary for now, ~ since we can't use <a href>
to give that native support). I've opted~ not to move this handling into a view for now, as it'd require more ~ state in more places. ~ The solution is the same as what we use for file attachment thumbnails
~ on a review request. We now have an <a href>
overlay that sits on top~ of the whole element and consumes all click events, giving us the ~ ability to navigate but also to respect browser link interactions. The ~ result is a wider clickable area for navigation. This has also been made ~ accessible. - Commits:
-
Summary ID a338d09ead7f5974745d5cefedd6ea09c450b5b4 2f80ff333b583534a5e906f317e87fa65ca16582