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

Review Request #13192 — Created Aug. 8, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

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

Description From Last Updated

We should probably change all of the ’ apostrophes to the ' ones that we normally use. And any “ …

maubinmaubin

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

maubinmaubin

Lets take out the word "providing" on this line to shorten the sentence a bit.

maubinmaubin

What's this addressing? Do other icons need it? Since we reuse this offset in &__close, can we add a constant …

chipx86chipx86

Will this tip show up outside of the review dialog? Should we give it a name that gives it context …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This needs to preventDefault() and stopPropagation().

chipx86chipx86

This is the inverse of the hide action. Maybe generalize this into a _toggleTips() and have both the event and …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86
david
maubin
  1. It actually fits quite nicely at the top of the dialog.

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

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

  4. Show all issues

    Lets take out the word "providing" on this line to shorten the sentence a bit.

  5. 
      
david
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. These tips are great!

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

  2. reviewboard/static/rb/css/ui/alert.less (Diff revision 4)
     
     
    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?

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

  3. 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?

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

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

  6. Show all issues

    This needs to preventDefault() and stopPropagation().

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

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

  8. 
      
david
chipx86
  1. One small docs thing.

  2. reviewboard/static/rb/css/ui/alert.less (Diff revision 5)
     
     
    Show all issues

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

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

  3. 
      
david
chipx86
  1. 
      
  2. reviewboard/static/rb/css/ui/alert.less (Diff revisions 5 - 6)
     
     
     
    Show all issues

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

  3. 
      
david
chipx86
  1. 
      
  2. reviewboard/static/rb/css/ui/alert.less (Diff revision 7)
     
     
     
     
    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.

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

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

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

  3. 
      
david
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (14ab0c8)