Unified Review Banner Base Shell

Review Request #10196 — Created Oct. 4, 2018 and updated

shoven
Review Board
release-4.0.x
a58aef7...
reviewboard, students

The Unified Review Banner Base Shell sets the foundation for the Unified
Review Banner, which will eventually take over for the three banners
currently used for draft review requests, reviews, and review replies.

The banner tracks the draft state of review requests, reviews, and
review replies so it can be put into a draft mode where the banner
appears green when at least one draft exists. The banner makes use of
RB.FloatingBannerView to allow the banner to float to the top of the
screen when the user scrolls down. The new RB.Registry model allows
views like dropdowns, buttons, and Backbone views to be registered in
different locations on the banner. There are placeholders registered to
the different banner sections right now which will later be removed or
replaced when the functionality of the Unified Review Banner is fleshed
out further.

Currently, the Unified Review Banner is only shown when the Unified
Banner feature flag is enabled, by appending the following code to
settings_local.py:

ENABLED_FEATURES = {
    'reviews.unified_banner': True
}

Added new JS tests to test functionality added. All JS tests pass.

Manual test cases:
1. Review request draft by modifying description, testing done,
information -> banner should turn green
2. Review request draft by adding a file (through drag-and-drop and
Update > Add File) -> banner should turn green
3. Discard review request draft with button on unified banner -> banner
should turn brown, review request discarded
4. New review by adding comment in diff viewer -> banner should turn
green
5. Discard review with button on unified banner -> banner should turn
brown, review discarded
6. New review with header, general comment in review dialog -> banner
should turn green
7. Discard review in review dialog -> banner should turn brown, review
discarded
8. Review reply draft by adding review replies on one review -> banner
should turn green
9. Remove all text of review reply draft (empty string) -> review reply
should be removed, banner should turn brown
10. Review reply draft by adding review replies on multiple/all reviews
-> banner should turn green
11. Discard review reply drafts with button on unified banner -> banner
should turn brown, all review replies discarded
12. New review/review request and review replies added -> banner should
turn green
13. Discard with button on unified banner -> banner should turn brown,
review and review replies discarded

*reload case (where on reload the correct banner state persists) tested
on all non-discard test cases

Loading file attachments...

  • 3
  • 0
  • 110
  • 4
  • 117
Description From Last Updated
To set the feature flag, I think you meant appending the code to settings.py bolariinwa bolariinwa
Instead of !!, can we just compare > 0? david david
I remember saying that we should use this.$el.append bolariinwa bolariinwa
brennie
  1. 
      
  2. You only need & if you want to specialize the parent selector e.g.:

    .foo {
        &.bar {
            // Equivalent to .foo.bar
        }
    }
    

    So you can just do > h1, > p { ....

    1. Will deal with this when I get to cleaning up the code this week or next week (this was actually just copying the existing .banner css so I could make changes without affecting the existing banners, but it will be revised).

  3. You only need _.defaults when combining objects. This can just be:

    defaults: {
        hasDraft: false,
    },
    
    1. I dropped this because I took this line out (wasn't really necessary at this point) but I will keep this in mind!

  4. Nit: space between if and (

  5. We only need the [].join('') in ES5 because it does not have multiline strings. You can just do:

    javascript: template: _.template(dedent` <h1><%- title %></h1> `),

    1. Dropped because I changed the way I was doing this.

  6. ES6 lets us do render(). Same below.

  7. This is going to run when the file is sourced, and so this object might not exist? We may want to store the default selector rather than the actual result of querying the selector. It also allows us to be lazy and only perform that query when necessary.

    1. Good catch. Do you think it would be useful to also refactor FloatingBannerView (which is what needs this property defined in the options) to just take in the selector in the options (like floatContainerSelector: '#container') and then just perform the query in that class when we actually use it?

    2. That sounds like a good plan!

    3. Okay, I'll think I'll do it in a separate review request because it looks like it will be a bit of work and I don't want it to make this one messy.

  8. this is undefined here. The second object passed to Backbone.{...}.extend is applied directly to the created object instead of its prototype.

    You will have to do:

    if (!RB.UnifiedReviewBannerView._instance) {
        // ...
    }
    
    1. Hmmm I did test this before I posted it and it worked as I expected so I tried console logging this here to double check and this is defined here. I did some digging on SO since the documentation didn't really get into the class properties (second object) passed to extend very much (the second answer here from mu is too short was helpful and describes why this didn't work in this poster's case for the static properties but it seems to work in my case). Is it preferable to use RB.UnifiedReviewBannerView._instance over this._instance even though both seem to work?

    2. So I was wrong. this is not undefined becuase it is being called with object.method(...) which is equivalent to object.method.call(object, ...).

      However, in this case this is not a model instance, instead it is the model itself, which may not be clear. So it may be case that we should do this for clarity, but maybe another mentor should weigh in?

      It is important to note that if you ever do something like:

      const getBanner = unifiedBannerEnabled ? RB.UnifiedBannerView.getInstance : RB.DraftBannerView.getInstance;
      
      const banner = getBanner();
      

      that this will not work because getBanner is being called without a context and so will get undefined instead of the context of its object.

  9. 
      
shoven
shoven
shoven
Review request changed
shoven
shoven
shoven
Sudolicious
  1. 
      
  2. Hey Sarah, Great work so far!
    I just wanted to clarify, I know that var is bound to the execution context, in this case the function. Wouldn't let be more appropriate?

    let banner,
        pendingReview,
        reviewRequest;
    

    Something similar to this^?

  3. Same thing as above, do these need to be declared with var?

  4. I'm only asking cause I thought we should avoid var entirely.

  5. I think this description is a bit of a mouthful.

    Would something like this be a more sufficient summary?
    "A unified, multi-mode banner that provides basic support for publishing/editing/discarding, batch operations and more."

  6. 
      
shoven
Sudolicious
  1. 
      
  2. You can use an arrow function here in order to avoid assigning const that = this;

    Since arrow functions don't have their own value of this, the _.each function can be rewritten as:

    _.each(defaults, item => {
      this.register(item);
    });
    
  3. 
      
shoven
skaefer143
  1. 
      
  2. Hey Sarah! Is this TODO for you to work on in your project, or is this for somebody in the future to work on?

    1. This TODO will stick around until the Unified Review Banner is finished as a reminder that that code can be deleted :)

  3. Hey Sarah! Is this TODO for you to work on in your project, or is this for somebody in the future to work on?

    1. This TODO will stick around until the Unified Review Banner is finished as a reminder that that code can be deleted :)

  4. This comment should be a full sentence, with a period.

  5. This comment should be a full sentence, with a period.

  6. This comment should be a full sentence, with a period.

  7. 
      
shoven
brennie
  1. 
      
  2. Can you add screenshots?

  3. This should be // TODO: Full sentence..

    Though I don't know that this is necessary.

  4. Same here.

  5. This should probably be #unified-banner-docked.

    Same for other selectors: they should spell out the entire phrase.

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

    Should be organized like:

    .selector {
        property: value;
    
        &-selectors {}
    
        .other-selectors {}
    }
    
  7. .selector * is super expensive.

    CSS selectors are applied right-to-left, so the * will match every DOM element and then the browser will check .selector against every parent.

  8. .selector * is super expensive. See other comment.

  9. This needs a corresponding afterEach() to reset the feature to its original value.

  10. Doc comments should be of the form:

    /**
     * Single line summary.
     *
     * Multi-line description.
     */
    
  11. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     

    Since reviewReplyDrafts is an Array, which is mutable, this needs to be a function:

    defaults() {
        return {
            hasDraft: false,
            // ...
        };
    },
    
  12. Why these? These are in the defaults.

  13. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    The arrow function should be aligned with the first argument.

    The arrow functino should be () => this._updateHasDraft(). Making it multiline is overkill.

  14. Again, this should just be hasDraft.

    Also, we can rewrite this using Backbone.Model.prototype.isNew().

    const hasDraft = (!this.get('reviewRequest').isNew() ||
                     !this.get('pendingReview').isNew() ||
                     !!this.get('reviewReplyDrafts').length)
    
  15. We are declaring a variable and then immediately using it once. We should move it inline.

  16. Doc comments should be of the format:

    /**
     * Single line summary.
     *
     * Multi-line description.
     */
    
  17. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     

    These arrow functions can be a single line.

  18. We don't do local vars with leading underscores.

  19. You are re-using this in closures. Is this intentional?

    1. Can you elaborate? Are you recommending I don't reuse it?

    2. Actually David's comment about this same thing clarified it for me :)

  20. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     

    This should be formatted as either:

    this.listenTo(reviewReply, 'saved',
                  () => {
                      // ...
                  });
    
    // or
    this.listenTo(
        reviewReply, 'saved',
        () => {
            // ...
        });
    

    (Probably the latter to avoid rightward drift.)

  21. Blank line between these.

  22. Instead of _.findWhere, why not:

    if (!_reviewReplyDrafts.includes(reviewReply))
    

    _.findWhere compares all attributes instead of just comparing for equality. Two models with the same ID will have the property that a == b.

  23. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     

    This can be rewritten as:

    this.listenTo(
        reviewReply, 'destroying',
        () => this.set('reviewReplyDrafts', _.reject(
            this.get('reviewReplyDrafts'),
            draft => draft.id === reviewReply.id)));
    
  24. Any reason you're not using arrow syntax?

    1. No, it seems that I momentarily forgot that arrow syntax was a thing :) Updated with your earlier suggestion that uses arrow syntax.

  25. This should be formatted as:

    this.listenTo(reviewReply, 'destroyed',
                  () => this._updateHasDraft());
    

    The { and } are unnecessary for a single-line lambda.

  26. Blank line between these.

  27. Doc comments should be of the format:

    /**
     * Single line summary.
     *
     * Multi-line description.
     */
    
  28. Blank line between these.

  29. Undo this formatting change.

  30. There should be a corresponding afterEach() that resets RB.EnabledFeatures.unifiedBanner.

  31. There should be a corresponding afterEach() that resets RB.EnabledFeatures.unifiedBanner.

  32. 
      
brennie
  1. 
      
  2. This should be a full sentence.

  3. 
      
shoven
david
  1. I have a bunch of comments, but they're almost all very simple formatting issues. Great work!

  2. "Attributes" should be capitalized here.

  3. Since this is implemented as a method, it should have a doc comment.

  4. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 10)
     
     
     
     
     
     
     
     
     

    This can be cleaned up a bit:

    _.bindAll(this, '_updateHasDraft');
    this.listenTo(reviewRequest.draft, 'saved destroy',
                  this._updateHasDraft);
    this.listenTo(pendingreview, 'saved destroy',
                  this._updateHasDraft);
    
  5. Instead of !!, can we just compare > 0?

    1. I originally had this.get('reviewReplyDrafts').length > 0 and then in one of Barret's comments he suggested !!this.get('reviewReplyDrafts').length so I changed it to that. I don't really have a preference either way but is one way preferred over the other for y'all?

  6. If you do the bindAll in initialize, you can just pass in ready: this._updateHasDraft, here.

  7. I think it would be better to have multiple const reviewReplyDrafts = ... in each place that it's used. Right now you share this same variable between the function and two inner closures, which seems ripe for strange bugs.

  8. Assuming reviewReplies is an array, we can probably just use !reviewReplies.includes(reviewReply) (I haven't tested that, though).

  9. There's a better way to wrap this:

    this.listenTo(reviewReply, 'saved', () => {
        reviewReplyDrafts = this.get('reviewReplyDrafts');
        ...
    });
    

    (saves one level of indentation and a couple lines)

  10. Same wrapping here as above.

  11. If you have the bindAll in initialize mentioned before, this can just pass in _updateHasDraft directly.

  12. Probably better to use

    this.get('reviewReplyDrafts').forEach(reviewReply => {
       ...
    });
    

    We're slowly replacing a lot of underscore use with modern JS features like that.

  13. You should be able to skip the truthy check here. isFunction will return false if it's undefined/null/etc.

  14. Please put a blank line between these two lines.

  15. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This is nested very deep. Can we break this stuff out into its own function, and then maybe define the registries in their own variable first before passing them into create()? Just to flatten the code a little bit.

  16. We should probably save the value inside beforeEach instead of here when the suite is defined.

  17. We should probably save the value inside beforeEach instead of here when the suite is defined.

  18. Only one blank line here.

  19. We should probably save the value inside beforeEach instead of here when the suite is defined.

  20. We should probably save the value inside beforeEach instead of here when the suite is defined.

  21. We should probably save the value inside beforeEach instead of here when the suite is defined.

  22. This is a regular JS file for now, so the last item in an object shouldn't have a trailing comma.

  23. Add this line back please.

  24. Add a blank line between these two.

  25. This can just be:

    this.listenTo(view, 'fieldSaved', this.showBanner);
    
  26. Add a blank line between these two.

  27. Better to use listenTo:

    this.listenTo(this.model, 'saved', this.showBanner);
    
  28. Alignment is wacky here. Should look like:

    ```javascript
    } else if (state === RB.ReviewRequest.PENDING &&
    this.model.get('hasDraft')) {
    BannerClass = DraftBannerView;
    }

  29. Should probably save the value inside beforeEach

  30. Should save the value inside beforeEach

  31. Unless we change this, it should be defined as const.

    This is also a great place to use dedent.

  32. Can you include an "Option Args" section here, explaining what can be in options?

  33. Should have a "Returns" section.

  34. We're moving away from _super because it's not reliable. Can you just use RB.FloatingBannerView.prototype.render.call?

  35. Formatting of this comment isn't correct. It should be one line summary, blank line, longer description, and then include an "Args" section explaining isDraft.

  36. Should have a one-line summary, then a blank line, then longer description.

  37. Please include an "Option Args" section.

  38. Can we line everything up after the (?

  39. Second line here should line up with the other !options in the conditional.

  40. "Attributes" should be capitalized.

  41. Type can be Backbone.View or string

  42. Add a blank line between these.

  43. Should just be jQuery for the type.

  44. Please include a doc comment here.

  45. If this is a backbone view (having el), it will also have a $el.

  46. "Attributes" should be capitalized.

  47. Indentation in these should be 4 spaces instead of 2.

  48. Needs a "Returns" section.

  49. This can use an ES6 template string:

    console.error(`Index (${index}) is out of range.`);
    
  50. Should have a summary/description format. You can probably just add a blank line after "Register an item."

  51. Indentation in these should be 4 spaces instead of 2.

  52. item isn't used outside of the loop, so I'd rather do:

    while (registryCollection.length > 0) {
        const item = registryCollection.pop();
        item.get('$view').remove();
    }
    
  53. Probably better to use forEach instead of _.each.

  54. Description should be on the next line after the type.

  55. Indentation should be 4 spaces, not 2.

  56. Should use summary/description format.

  57. Please add a doc comment here.

  58. This can be elided, since it doesn't do anything.

  59. 
      
shoven
bolariinwa
Sudolicious
  1. 
      
  2. Hey Sarah, I was wondering if the {} are redundant over here?
    So would it be
    () => RB.DraftReviewBannerView.instance.show());?

  3. 
      
shoven
Review request changed
bolariinwa
  1. 
      
  2. Is it possible to state the type of object? From the comment describing registry I assume that defaultItems contains views

  3. 
      
bolariinwa
  1. 
      
  2. To set the feature flag, I think you meant appending the code to settings.py

  3. 
      
bolariinwa
  1. 
      
  2. I remember saying that we should use this.$el.append

    1. My bad I meant to say "I remember Barret saying"

  3. 
      
Loading...