Fix review dialog tips display and make some sizing/a11y tweaks.

Review Request #13245 — Created Aug. 29, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

This change has two pieces:

First, the tips in the review dialog weren't showing correctly because
somewhere along the line I lost the change that actually imports the CSS
for slideshow elements into the common bundle.

Second, the previous, next, and close buttons were all somewhat hard to
hit, and were missing some attributes needed for accessibility.

  • Verified that the tips UI worked correctly.
  • Checked the hit boxes and a11y attributes for the various buttons in
    the box.
Summary ID
Fix review dialog tips display and make some sizing/a11y tweaks.
This change has two pieces: First, the tips in the review dialog weren't showing correctly because somewhere along the line I lost the change that actually imports the CSS for slideshow elements into the common bundle. Second, the previous, next, and close buttons were all somewhat hard to hit, and were missing some attributes needed for accessibility. Testing Done: - Verified that the tips UI worked correctly. - Checked the hit boxes and a11y attributes for the various buttons in the box.
54d47f6c9dc5538fb4ee1d8c79992e5bf719bc37
Description From Last Updated

We still want aria-label. That's distinct from title (which will be used for tooltips but not as a replacement label …

chipx86chipx86

Alphabetical order. aria-label is needed.

chipx86chipx86

0.25em still seems small. We need to make sure this is touch-friendly. Can you post a screenshot?

chipx86chipx86

Should we use _ instead of gettext? Here and below.

maubinmaubin
maubin
  1. 
      
  2. Should we use _ instead of gettext? Here and below.

    1. We can't use _ because that relies on JS template strings with backticks, and this whole thing is inside one of those.

    2. Oh I see.

  3. 
      
maubin
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/css/ui/alert.less (Diff revision 1)
     
     
     
     
     
     
     
     
     

    We still want aria-label. That's distinct from title (which will be used for tooltips but not as a replacement label for screen readers).

  3. reviewboard/static/rb/css/ui/slideshow.less (Diff revision 1)
     
     
     
     
     
     

    Alphabetical order.

    aria-label is needed.

  4. 0.25em still seems small. We need to make sure this is touch-friendly. Can you post a screenshot?

    1. Any bigger than this felt really weird.

      Image

    2. I know the rest of the UI isn't built for mobile, but maybe an option is to increase the padding only when we detect a touchscreen.

      CSS offers a way to do this:

      @media (pointer: coarse) {
          ...
      }
      
    3. I think at this point I'd prefer to go with what I have here and I can play with these for touch for RC.

  5. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (d0d71d7)
Loading...