Improve accessibility and add dark mode for comment issue bars.
Review Request #13715 — Created April 8, 2024 and submitted
The comment issue bar has been rewritten entirely to be accessible, to
support dark mode, and to be more performant.The CSS side is now using our CSS Component standards, and makes use of
CSS variables and CSS grid to do the layout. Through this, we now have
dark mode support, CSS-managed status icons, and an improved mobile UI.The JavaScript side has been rewritten in TypeScript and Ink. It no
longer uses the old behavior of defining every button up-front and
toggling visibility, and instead manages the list of buttons relevant to
the current issue and user states. This, combined with ARIA information,
gives us a more accessible and clean bar.It also makes use of the new per-comment issue events, which means that
toggling an issue no longer results in every single issue bar on the
page running checks. It's faster and less error-prone than what we had
before.In dark mode, we now go for a layered grey background, and use the icon,
text, and button colors to help sell the appearance of the bar. The text
and buttons are now yellow-themed, like the bar is in light mode.The buttons also now show an animation when in the middle of setting a
new issue status. This leveragesInk.Button
's busy state.
All unit tests pass.
Tested all the issue status combinations, making sure that loading and
changing statuses worked as expected.Tested dark and light modes, desktop and mobile.
Checked the accessibility tree for any apparant issues.
Summary | ID |
---|---|
4b12b64c9af22c7325253b88164898dfbeb42b06 |
Description | From | Last Updated |
---|---|---|
Can we swap out the _.each for for (const issueBarEl of this._$reviewComments.find(...)) { ? |
david | |
Can be type BaseComponentViewOptions |
david | |
Can be type EventsHash |
david | |
These can all be import type |
david | |
object -> IssueStatusUpdatedEventData |
david | |
This won't be necessary after /r/13714/ |
david | |
This should be newIssueStatus, and I'd prefer to use CommentIssueStatusType instead of string. |
maubin | |
Missing docs for issueStatus, also can give it a type of CommentIssueStatusType instead of string. |
maubin |
Change Summary:
Added missing CSS changes and a fix to the template.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+3030 -1802) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/reviewRequestPage/views/reviewView.es6.js (Diff revision 2) Can we swap out the
_.each
forfor (const issueBarEl of this._$reviewComments.find(...)) {
? -
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 2) Can be
type BaseComponentViewOptions
-
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 2) Can be
type EventsHash
-
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 2) These can all be import type
-
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 2) object -> IssueStatusUpdatedEventData
-
reviewboard/static/rb/js/reviews/views/tests/commentIssueBarViewTests.ts (Diff revision 2) This won't be necessary after /r/13714/
Change Summary:
- Switched a legacy
_.each()
to afor..of
. - Imported some things as
type
. - Removed the now-unnecessary
$testsScratch
declaration.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+3030 -1804) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 3) This should be
newIssueStatus
, and I'd prefer to useCommentIssueStatusType
instead ofstring
. -
reviewboard/static/rb/js/reviews/views/commentIssueBarView.ts (Diff revision 3) Missing docs for
issueStatus
, also can give it a type ofCommentIssueStatusType
instead ofstring
.
Change Summary:
- Added types to docs.
- Added missing args in docs.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+3038 -1804) |