Summary

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

Review Request #13245 — Created Aug. 29, 2023 and submitted Aug. 29, 2023, 6:17 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Commits

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

Summary

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

Issues

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 …	chipx86	Aug. 29, 2023, 2:12 p.m.
Alphabetical order. aria-label is needed.	chipx86	Aug. 29, 2023, 2:13 p.m.
0.25em still seems small. We need to make sure this is touch-friendly. Can you post a screenshot?	chipx86	Aug. 29, 2023, 5:13 p.m.
Should we use _ instead of gettext? Here and below.	maubin	Aug. 29, 2023, 10:30 a.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 1)

The issue has been dropped. Show all issues

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

david Aug. 29, 2023, 10:29 a.m.

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

maubin Aug. 29, 2023, 10:30 a.m.
```
Oh I see.
```

Ship it!

```
Ship It!
```

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

The issue has been resolved. Show all issues

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).

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

The issue has been resolved. Show all issues

Alphabetical order.

aria-label is needed.

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

The issue has been dropped. Show all issues

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

david Aug. 29, 2023, 2:14 p.m.

Any bigger than this felt really weird.

chipx86 Aug. 29, 2023, 3:41 p.m.

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) {
    ...
}

david Aug. 29, 2023, 4:36 p.m.

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.

Commits:

	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.	20a2cbdab243a4e51d633b57f05e593f0012c79b
	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

Diff:

Revision 2 (+108 -16)

Show changes

	reviewboard/static/rb/css/bundles/common.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/alert.less
	reviewboard/static/rb/css/ui/slideshow.less
	reviewboard/static/rb/js/reviews/views/reviewDialogView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-6.x (d0d71d7)