Summary

Make a bunch of improvements to the tips in the review dialog.

Review Request #13192 — Created Aug. 8, 2023 and submitted Aug. 25, 2023, 9:40 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change makes a bunch of improvements to the new tips feature in the
review dialog:

Adds a hide button to allow people to hide the tips. While it's
probably good for new users, experienced users likely won't want it
cluttering up their screen. The preference for this is stored in a
cookie
Moves the tips box to the top of the review dialog.
Changes it so the tips box is always visible, even when there's review
content.
Adds new content to the tips list (thanks Michelle!).

Testing Done


Toggled the tips view back and forth.
Checked that the tips view shows up in the right place.
Verified that the initial tips view visibility respects the stored

  preference.
Ran unit tests.
Ran js-tests.

Commits

Summary	ID
Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	d6eaa6f1eb0337739d051a3e68a0a4a1489f5407

Summary

Make a bunch of improvements to the tips in the review dialog.

This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.

d6eaa6f1eb0337739d051a3e68a0a4a1489f5407

Files

Issues

Description	From	Last Updated
We should probably change all of the ’ apostrophes to the ' ones that we normally use. And any “ …	maubin	Aug. 9, 2023, 4:51 p.m.
Here we say review banner but in the "Ship It!" tip we say Review menu. We should standardize on one …	maubin	Aug. 9, 2023, 4:51 p.m.
Lets take out the word "providing" on this line to shorten the sentence a bit.	maubin	Aug. 9, 2023, 4:52 p.m.
What's this addressing? Do other icons need it? Since we reuse this offset in &__close, can we add a constant …	chipx86	Aug. 11, 2023, 5:02 p.m.
Will this tip show up outside of the review dialog? Should we give it a name that gives it context …	chipx86	Aug. 11, 2023, 5:03 p.m.
Let's pull out the full manual URL to a variable, so that localized strings don't hard-code any parts of this …	chipx86	Aug. 11, 2023, 5:08 p.m.
We may want to add role="button" to help screen readers know this is an action and not something that'll take …	chipx86	Aug. 11, 2023, 5:06 p.m.
This needs to preventDefault() and stopPropagation().	chipx86	Aug. 11, 2023, 5:07 p.m.
This is the inverse of the hide action. Maybe generalize this into a _toggleTips() and have both the event and …	chipx86	Aug. 11, 2023, 5:07 p.m.
Not sure how the <p> and <span> relate here. The <p> is self-closing, but the <span> indents.	chipx86	Aug. 24, 2023, 10:20 p.m.
Can you add a Structure: to this and also include this in the main element's Structure:?	chipx86	Aug. 23, 2023, 4:31 a.m.
This should still have a Structure, even if it's a simple standalone already shown elsewhere. All parts of all CSS …	chipx86	Aug. 25, 2023, 5:32 p.m.

flake8 passed.

JSHint passed.

Summary:

-	Make review dialog tips hide-able.
+	Make a bunch of improvements to the tips in the review dialog.

Description:

~		The new tips alert on the review dialog is intended to make things
~		easier for first-time users, but might be annoying for people who have
	~	This change makes a bunch of improvements to the new tips feature in the
	~	review dialog:
-		been using Review Board forever. It's already pretty unobtrusive,
-		because it only shows up when the review dialog is empty, but it's still
-		visual noise that people might not want to have.

~		This change adds a toggle that lets users hide it. The preference for
~		that is stored in a cookie, much the same way as the extra whitespace
~		for diffs.
	~	Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie
	~	Moves the tips box to the top of the review dialog.
	~	Changes it so the tips box is always visible, even when there's review content.
	+	Adds new content to the tips list (thanks Michelle!).

Testing Done:

		Toggled the tips view back and forth.
~		Verified that nothing appears when there's content in the review.
~		Ran unit tests.
~		Ran js-tests.
	~	Checked that the tips view shows up in the right place.
	~	Verified that the initial tips view visibility respects the stored preference.
	~	Ran unit tests.
	+	Ran js-tests.

Commits:

	Summary	ID
	Make review dialog tips hide-able. The new tips alert on the review dialog is intended to make things easier for first-time users, but might be annoying for people who have been using Review Board forever. It's already pretty unobtrusive, because it only shows up when the review dialog is empty, but it's still visual noise that people might not want to have. This change adds a toggle that lets users hide it. The preference for that is stored in a cookie, much the same way as the extra whitespace for diffs. Testing Done: - Toggled the tips view back and forth. - Verified that nothing appears when there's content in the review. - Ran unit tests. - Ran js-tests.	992315c431adcfdb81985f933726313a27930438
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	5d725dabc3350f4920efe9008147e86a930aa55d

Diff:

Revision 2 (+236 -26)

Show changes

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

Added Files:

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

It actually fits quite nicely at the top of the dialog.

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

The issue has been resolved. Show all issues

We should probably change all of the ’ apostrophes to the ' ones that we normally use. And any “ quotes to ". I'm guessing the other ones showed up just from copying text from Slack.

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

The issue has been resolved. Show all issues

Here we say review banner but in the "Ship It!" tip we say Review menu. We should standardize on one (probably Review menu).

reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Lets take out the word "providing" on this line to shorten the sentence a bit.
```

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	5d725dabc3350f4920efe9008147e86a930aa55d
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	65406cd4f041d24651972bdab77a59f2b4a7722d

Diff:

Revision 3 (+234 -26)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	65406cd4f041d24651972bdab77a59f2b4a7722d
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	4e9deaaed3f9da5cee75fa84f2b8171beff4df09

Diff:

Revision 4 (+234 -26)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

These tips are great!

Change overall looks great, too. A few fairly small things.

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

The issue has been resolved. Show all issues

What's this addressing? Do other icons need it?

Since we reuse this offset in &__close, can we add a constant or combine those two rules?

david Aug. 11, 2023, 5:10 p.m.

This was a mistake. I was intending to add this only to __close and it just ended up in the wrong spot.

reviewboard/static/rb/js/common/models/userSessionModel.ts (Diff revision 4)

The issue has been resolved. Show all issues

Will this tip show up outside of the review dialog? Should we give it a name that gives it context for the review dialog, in case we want to add tips to the review request page or something down the road?

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

The issue has been resolved. Show all issues

Let's pull out the full manual URL to a variable, so that localized strings don't hard-code any parts of this URL.

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

The issue has been resolved. Show all issues

We may want to add role="button" to help screen readers know this is an action and not something that'll take them elsewhere.

reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 4)
The issue has been resolved. Show all issues
```
This needs to preventDefault() and stopPropagation().
```

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

The issue has been resolved. Show all issues

This is the inverse of the hide action. Maybe generalize this into a _toggleTips() and have both the event and an events entry for this tie into that?

david Aug. 11, 2023, 5:10 p.m.

I actually meant to delete the show/hide calls here since that's what #updateTipsVisibility does.

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	4e9deaaed3f9da5cee75fa84f2b8171beff4df09
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	c1ca063f2cd582833e64ba1c3ef2a6107e9375a4

Diff:

Revision 5 (+228 -28)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
One small docs thing.
```

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

The issue has been resolved. Show all issues

Can you add a Structure: to this and also include this in the main element's Structure:?

david Aug. 23, 2023, 4:32 a.m.

This is always just an empty <span> but I'll add it to the main structure.

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	c1ca063f2cd582833e64ba1c3ef2a6107e9375a4
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	ff162f929a04f9e4c0de330634da8dc10fd4ba52

Diff:

Revision 6 (+230 -28)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/css/ui/alert.less (Diff revisions 5 - 6)

The issue has been resolved. Show all issues

Not sure how the <p> and <span> relate here. The <p> is self-closing, but the <span> indents.

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	ff162f929a04f9e4c0de330634da8dc10fd4ba52
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	33cb0d2d961d7e7d2e5d84cb4d6a5aab0750726e

Diff:

Revision 7 (+230 -28)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

This should still have a Structure, even if it's a simple standalone already shown elsewhere. All parts of all CSS components should have this.

david Aug. 24, 2023, 10:25 p.m.

But there's no structure. It's just an empty <span> element with no children.

chipx86 Aug. 24, 2023, 11:48 p.m.

The structure contains the tag the part applies to and any applicable children. There's zero in this case, but we still document the tag itself in all other cases.

And actually, in this case, it catches something. A <span> isn't right for a button. We need this to be either a <button> itself, or use role="button" aria-label="..." tabindex="0", to be accessible. The docs should describe and show that.

chipx86 Aug. 24, 2023, 11:49 p.m.

(Those should also be listed in "DOM Attributes").

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	33cb0d2d961d7e7d2e5d84cb4d6a5aab0750726e
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	375fcf151a456654b8b7f3ecdd7cb759782d14fe

Diff:

Revision 8 (+244 -28)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	375fcf151a456654b8b7f3ecdd7cb759782d14fe
	Make a bunch of improvements to the tips in the review dialog. This change makes a bunch of improvements to the new tips feature in the review dialog: - Adds a hide button to allow people to hide the tips. While it's probably good for new users, experienced users likely won't want it cluttering up their screen. The preference for this is stored in a cookie - Moves the tips box to the top of the review dialog. - Changes it so the tips box is always visible, even when there's review content. - Adds new content to the tips list (thanks Michelle!). Testing Done: - Toggled the tips view back and forth. - Checked that the tips view shows up in the right place. - Verified that the initial tips view visibility respects the stored preference. - Ran unit tests. - Ran js-tests.	d6eaa6f1eb0337739d051a3e68a0a4a1489f5407

Diff:

Revision 9 (+262 -28)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (14ab0c8)