Add the new unified banner.

Review Request #12734 — Created Nov. 22, 2022 and submitted

david
Review Board
release-6.x
reviewboard

This change adds the unified banner, a major new UI feature in Review
Board 6.

This new banner replaces several existing banners:

  • The review request draft banner
  • The pending review banner
  • Review reply banners (at least the ones that float)

The goal here is twofold. First, we want to make it very clear whenever
there's any unpublished work (often review reply drafts can get lost in
long pages). Second, we want to enable people to publish everything all
in one go (the typical use case being publishing a bunch of review
replies and a review request update all together).

This also gives us room for future work:

  • We have space at the right hand side of the banner for a future
    block showing info about the current review, popping up into a
    redesigned review dialog.
  • We have a "dock" area (currently empty, but present in the DOM and
    properly styled), which we intend to use for a quick file switcher in
    the diffviewer, and possibly other things.

This is mostly complete. The current things left on my to-do list are:

  • Further testing of the banner in mobile mode. Right now interaction
    with the menus has some problems but it's possible that's just
    Firefox's devtools.
  • Possibly moving the "Add General Comment" action into the review menu.
  • Add more js-tests.
  • Ran js-tests.
  • Ran python unit tests.
  • Tested all different combinations of review requset changes, reviews,
    and review replies. Saw that the draft mode selector showed the
    correct options.
  • Tested publishing with individual items and as a batch.
  • Tested discard with various individual items.
  • Tested the change description field.
  • Tested the new actions (create review, edit review, ship it) that live
    in the "Review" menu.
Summary
Add the new unified banner.

Description From Last Updated

'unifiedBanner' is defined but never used. Column: 15 Error code: W098

reviewbotreviewbot

Duplicate key 'click #action-add-general-comment'. Column: 44 Error code: W075

reviewbotreviewbot

This is probably true of other previously-reviewed versions as well, but looking more closely at it, this text is really …

chipx86chipx86

Missing period.

chipx86chipx86

Can we keep this in alphabetical order?

chipx86chipx86

Thinking it might be worth fleshing this out a bit. There's a lot going on in this banner, so just …

chipx86chipx86

Tiny nit: Can we have a blank line between multi-line list items? Helps keep it all from visually blending together …

chipx86chipx86

This line is too long.

chipx86chipx86

This is missing a doc comment.

chipx86chipx86

Ordering is off. Any ID-based selectors should go after the &__ parts.

chipx86chipx86

&__mode should have a doc comment somewhere, even if the ruleset is empty. This will be important for the doc …

chipx86chipx86

Missing a doc comment.

chipx86chipx86

Modifier rules should go before any child rules, up near the top, since they apply to the main component.

chipx86chipx86

This could go in &__menu (though you would need to replace the &__mode with the full name). Might be a …

chipx86chipx86

Let's standardize on a blank line after this comment, so there's always room/incentive for per-variable comments. In this case, though, …

chipx86chipx86

Can we say how this will update it?

chipx86chipx86

These are missing docs.

chipx86chipx86

Can we use numDrafts or draftCount instead? The n prefix has always felt too much like shorthand to me.

chipx86chipx86

Same comment about the n prefix.

chipx86chipx86

Can we have a comment describing this particular condition? There's a lot to keep track of in these. Might be …

chipx86chipx86

Can we add a file-level doc comment?

chipx86chipx86

Wonder if we should treat other bundles as a separate import group from stuff in the current bundle. Imports in …

chipx86chipx86

Missing a Version Added. Here and below.

chipx86chipx86

Let's standardize on a consistent blank line after the "Instance variables" block. I also realized, from working with the AST …

chipx86chipx86

These should be in alphabetical order.

chipx86chipx86

Completely optional (mostly using this comment to point it out the capability), but we do have this now: this.#menuView.renderInto(this.$el); Instead …

chipx86chipx86

This event handler isn't returning anything.

chipx86chipx86

Let's stop defaults/propagation here and below before we do anything else, so errors don't cause events to bubble.

chipx86chipx86

Maybe worth adding a clearItems() method to MenuView? I feel like this shouldn't be reaching into internals, or we're asking …

chipx86chipx86

We can probably skip this check, since if it's 0, the for loop is a no-op.

chipx86chipx86

Rather than querying this every time we update, let's pull this out during render and just access it via a …

chipx86chipx86

We fetch draftModes[i].text in both conditions, so we could just pull it into a variable once. That also simplifies the …

chipx86chipx86

Let's pull this out as well.

chipx86chipx86

There's a space between change: and draftModes here. Might need more testing on that.

chipx86chipx86

Blank line here.

chipx86chipx86

Missing a doc comment.

chipx86chipx86

The summary must be one line.

chipx86chipx86

This can use :js:attr

chipx86chipx86

We should use the specific element for the banner. Is that just a div?

chipx86chipx86

Missing space between > and {.

chipx86chipx86

Can we type this?

chipx86chipx86

There will be things to figure out with some of the references cross-bundle, but I think you want: :js:class:`RB.FloatingBannerView`

chipx86chipx86

No return value needed here.

chipx86chipx86

No need to call the parent. That's explicitly empty.

chipx86chipx86

If any of these are direct children, let's use this.$el.children('...') to avoid the deep lookup. Same elsewhere.

chipx86chipx86

We access this.model a bunch. Let's pull it out into a local variable.

chipx86chipx86

This can be: this.#modeMenu.renderInto(this.#modeSelector);

chipx86chipx86

Can we separate these? Keep the publish button stuff together?

chipx86chipx86

No return value needed here.

chipx86chipx86

If we're going to log, let's be more explicit about what wasn't rendered, since the log output won't have context. …

chipx86chipx86

Instead of repeated attribute lookups, let's pull out this.model into a local variable.

chipx86chipx86

Can we use numDrafts or draftCount instead of the n prefix?

chipx86chipx86

Since we're already using setVisible, this could be: this.#discardButton.setVisible( draftModes.length > 0 && !draftModes[selectedDraftMode].multiple);

chipx86chipx86

We should add an interface for this.

chipx86chipx86

Let's pull out this.model into a local variable.

chipx86chipx86

Can we type these?

chipx86chipx86

For this call, let's have two groups of keys (separated by a blank line): Options/data for the API call Callbacks …

chipx86chipx86

We access this.model a lot in this function, so let's pull it out.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

david
Review request changed

Summary:

-[WIP] Add the new unified banner.
+Add the new unified banner.

Description:

~  

This is a very work-in-progress change to add the new unified banner.

~   Posting just for initial feedback on the broad strokes, with the
~   understanding that there's still a fair bit of work left to do on
~   everything.

  ~

This change adds the unified banner, a major new UI feature in Review

  ~ Board 6.

  ~
  ~

This new banner replaces several existing banners:

  +
  +
  • The review request draft banner
  +
  • The pending review banner
  +
  • Review reply banners (at least the ones that float)
  +
  +

The goal here is twofold. First, we want to make it very clear whenever

  + there's any unpublished work (often review reply drafts can get lost in
  + long pages). Second, we want to enable people to publish everything all
  + in one go (the typical use case being publishing a bunch of review
  + replies and a review request update all together).

  +
  +

This also gives us room for future work:

  +
  +
  • We have space at the right hand side of the banner for a future
    block showing info about the current review, popping up into a
    redesigned review dialog.
  +
  • We have a "dock" area (currently empty, but present in the DOM and
    properly styled), which we intend to use for a quick file switcher in
    the diffviewer, and possibly other things.
  +
  +

This is mostly complete. The current things left on my to-do list are:

  +
  +
  • Further testing of the banner in mobile mode. Right now interaction
    with the menus has some problems but it's possible that's just
    Firefox's devtools.
  +
  • Possibly moving the "Add General Comment" action into the review menu.

Testing Done:

  +
  • Ran js-tests.
  +
  • Ran python unit tests.
  +
  • Tested all different combinations of review requset changes, reviews,
    and review replies. Saw that the draft mode selector showed the
    correct options.
  +
  • Tested publishing with individual items and as a batch.
  +
  • Tested discard with various individual items.
  +
  • Tested the change description field.
  +
  • Tested the new actions (create review, edit review, ship it) that live
    in the "Review" menu.

Commits:

Summary
-
[WIP] Add the new unified banner.
+
Add the new unified banner.

Diff:

Revision 2 (+3592 -234)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
david
chipx86
  1. This is awesome, David. I'm so happy to see this becoming reality.

    There's a lot of comments here, but most are in the categories of:

    1. Doc improvements/missing docs
    2. Couple repeated naming suggestions
    3. Reducing unnecessary attribute accesses
    4. Missing types
    5. Just small typos and such

    The core of it looks really good. Nothing major anywhere, just polish. I can't wait to use this :)

  2. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     
     

    This is probably true of other previously-reviewed versions as well, but looking more closely at it, this text is really more about the base class's version and less about this specific implementation. Can you update it to describe how this class determines if the menu should be rendered?

  3. reviewboard/reviews/actions.py (Diff revision 3)
     
     
     

    Missing period.

  4. Can we keep this in alphabetical order?

  5. Thinking it might be worth fleshing this out a bit. There's a lot going on in this banner, so just having an overview of how it all fits together will help keep this maintainable.

    1. Will spend some time composing something that we can use everywhere.

  6. This line is too long.

  7. This is missing a doc comment.

  8. reviewboard/static/rb/css/ui/banners.less (Diff revision 3)
     
     
     
     

    Ordering is off. Any ID-based selectors should go after the &__ parts.

  9. reviewboard/static/rb/css/ui/banners.less (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    &__mode should have a doc comment somewhere, even if the ruleset is empty.

    This will be important for the doc generation stuff I'm presently toying with.

  10. reviewboard/static/rb/css/ui/banners.less (Diff revision 3)
     
     
     
     
     

    Missing a doc comment.

  11. reviewboard/static/rb/css/ui/banners.less (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Modifier rules should go before any child rules, up near the top, since they apply to the main component.

  12. Where does pending come from?

    1. It's what jasmine uses to skip a test.

  13. Let's standardize on a blank line after this comment, so there's always room/incentive for per-variable comments.

    In this case, though, I don't know that template is strictly an instance variable. I mean, it is, but in much the same way that className is. I think this section is best left for things that are set/mutated during the lifetime of an instance, not what are effectively constants.

  14. Can we say how this will update it?

  15. reviewboard/static/rb/js/reviews/models/unifiedBanner.ts (Diff revision 3)
     
     
     
     
     
     
     

    These are missing docs.

  16. Can we use numDrafts or draftCount instead? The n prefix has always felt too much like shorthand to me.

  17. Same comment about the n prefix.

  18. Can we have a comment describing this particular condition? There's a lot to keep track of in these.

    Might be nice to actually have one per-condition.

  19. Can we add a file-level doc comment?

  20. Wonder if we should treat other bundles as a separate import group from stuff in the current bundle.

    Imports in JavaScript feel so much more difficult to clearly organize. I haven't really found a really nice pattern I like, except that I've been loosely doing:

    1. Platform (for Node)
    2. Third-party (node_modules)
    3. Current bundle

    And I could see a 2.5 in there that's "other bundles in same project." Or maybe that just goes into third-party. I don't know. Thoughts?

    1. Sure, having a separate group for anything that's from 'reviewboard/...' makes sense to me. I guess djblets imports go with other third-party even though it's not node_modules?

  21. Missing a Version Added.

    Here and below.

  22. Let's standardize on a consistent blank line after the "Instance variables" block.

    I also realized, from working with the AST for TypeScript this past week, that this will prevent #menuView from being assigned that comment block as its leading comment.

  23. Completely optional (mostly using this comment to point it out the capability), but we do have this now:

    this.#menuView.renderInto(this.$el);
    

    Instead of the .append() and .render().

  24. This event handler isn't returning anything.

  25. reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3)
     
     
     
     
     
     
     
     

    Let's stop defaults/propagation here and below before we do anything else, so errors don't cause events to bubble.

  26. Maybe worth adding a clearItems() method to MenuView? I feel like this shouldn't be reaching into internals, or we're asking for trouble down the road.

  27. We can probably skip this check, since if it's 0, the for loop is a no-op.

  28. Rather than querying this every time we update, let's pull this out during render and just access it via a variable. We update more than we render, especially since this is in a loop.

  29. We fetch draftModes[i].text in both conditions, so we could just pull it into a variable once. That also simplifies the template literal.

  30. Let's pull this out as well.

  31. There's a space between change: and draftModes here. Might need more testing on that.

    1. I think it ended up triggering on any model change. Will double check functionality after fixing this.

  32. Missing a doc comment.

  33. The summary must be one line.

    1. This is from a very old version of the change. Deleting.

  34. We should use the specific element for the banner. Is that just a div?

  35. Missing space between > and {.

  36. There will be things to figure out with some of the references cross-bundle, but I think you want:

    :js:class:`RB.FloatingBannerView`
    
  37. No return value needed here.

  38. No need to call the parent. That's explicitly empty.

  39. If any of these are direct children, let's use this.$el.children('...') to avoid the deep lookup.

    Same elsewhere.

  40. We access this.model a bunch. Let's pull it out into a local variable.

  41. This can be:

    this.#modeMenu.renderInto(this.#modeSelector);
    
  42. Can we separate these? Keep the publish button stuff together?

  43. No return value needed here.

  44. If we're going to log, let's be more explicit about what wasn't rendered, since the log output won't have context.

    Or should this be an assert at this point?

    1. This was old debug code.

  45. Instead of repeated attribute lookups, let's pull out this.model into a local variable.

  46. Can we use numDrafts or draftCount instead of the n prefix?

  47. reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3)
     
     
     
     
     
     
     

    Since we're already using setVisible, this could be:

    this.#discardButton.setVisible(
        draftModes.length > 0 &&
        !draftModes[selectedDraftMode].multiple);
    
  48. We should add an interface for this.

    1. My feeling on these is that it's worth adding an interface when we use it in more than one spot.

  49. Let's pull out this.model into a local variable.

  50. For this call, let's have two groups of keys (separated by a blank line):

    1. Options/data for the API call
    2. Callbacks

    That'll help keep this a lot more readable while also keeping ESLint happy.

  51. We access this.model a lot in this function, so let's pull it out.

  52. 
      
david
david
maubin
  1. Everything looks great! So excited to use this.

  2. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/css/ui/banners.less (Diff revisions 3 - 5)
     
     
     

    Tiny nit: Can we have a blank line between multi-line list items? Helps keep it all from visually blending together as much.

  3. reviewboard/static/rb/css/ui/banners.less (Diff revisions 3 - 5)
     
     
     
     
     
     
     
     
     
     

    This could go in &__menu (though you would need to replace the &__mode with the full name). Might be a bit better just for organization.

    1. I think I prefer it as-is, since it's really a modification of the __mode element that was just defined.

  4. These should be in alphabetical order.

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

Status: Closed (submitted)

Change Summary:

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