Unified Review Banner Base Shell

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

Information

Review Board
master

Reviewers

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


Description From Last Updated

Can you add screenshots?

brenniebrennie

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

bolariinwabolariinwa

You only need & if you want to specialize the parent selector e.g.: .foo { &.bar { // Equivalent to …

brenniebrennie

You only need _.defaults when combining objects. This can just be: defaults: { hasDraft: false, },

brenniebrennie

Nit: space between if and (

brenniebrennie

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

brenniebrennie

ES6 lets us do render(). Same below.

brenniebrennie

This is going to run when the file is sourced, and so this object might not exist? We may want …

brenniebrennie

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

brenniebrennie

Col: 15 'pendingReview' is defined but never used.

reviewbotreviewbot

Hey Sarah, Great work so far! I just wanted to clarify, I know that var is bound to the execution …

SudoliciousSudolicious

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

SudoliciousSudolicious

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

SudoliciousSudolicious

I think this description is a bit of a mouthful. Would something like this be a more sufficient summary? "A …

SudoliciousSudolicious

Hey Sarah! Is this TODO for you to work on in your project, or is this for somebody in the …

skaefer143skaefer143

Hey Sarah! Is this TODO for you to work on in your project, or is this for somebody in the …

skaefer143skaefer143

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

skaefer143skaefer143

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

skaefer143skaefer143

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

skaefer143skaefer143

You can use an arrow function here in order to avoid assigning const that = this; Since arrow functions don't …

SudoliciousSudolicious

This should be // TODO: Full sentence.. Though I don't know that this is necessary.

brenniebrennie

Same here.

brenniebrennie

This should probably be #unified-banner-docked. Same for other selectors: they should spell out the entire phrase.

brenniebrennie

Should be organized like: .selector { property: value; &-selectors {} .other-selectors {} }

brenniebrennie

.selector * is super expensive. CSS selectors are applied right-to-left, so the * will match every DOM element and then …

brenniebrennie

.selector * is super expensive. See other comment.

brenniebrennie

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

brenniebrennie

Doc comments should be of the form: /** * Single line summary. * * Multi-line description. */

brenniebrennie

Since reviewReplyDrafts is an Array, which is mutable, this needs to be a function: defaults() { return { hasDraft: false, …

brenniebrennie

Missing doc comment.

brenniebrennie

Why these? These are in the defaults.

brenniebrennie

The arrow function should be aligned with the first argument. The arrow functino should be () => this._updateHasDraft(). Making it …

brenniebrennie

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() || …

brenniebrennie

We are declaring a variable and then immediately using it once. We should move it inline.

brenniebrennie

Doc comments should be of the format: /** * Single line summary. * * Multi-line description. */

brenniebrennie

These arrow functions can be a single line.

brenniebrennie

Missing a summary.

brenniebrennie

We don't do local vars with leading underscores.

brenniebrennie

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

brenniebrennie

This should be formatted as either: this.listenTo(reviewReply, 'saved', () => { // ... }); // or this.listenTo( reviewReply, 'saved', () …

brenniebrennie

Blank line between these.

brenniebrennie

Instead of _.findWhere, why not: if (!_reviewReplyDrafts.includes(reviewReply)) _.findWhere compares all attributes instead of just comparing for equality. Two models with …

brenniebrennie

This can be rewritten as: this.listenTo( reviewReply, 'destroying', () => this.set('reviewReplyDrafts', _.reject( this.get('reviewReplyDrafts'), draft => draft.id === reviewReply.id)));

brenniebrennie

Any reason you're not using arrow syntax?

brenniebrennie

This should be formatted as: this.listenTo(reviewReply, 'destroyed', () => this._updateHasDraft()); The { and } are unnecessary for a single-line lambda.

brenniebrennie

Blank line between these.

brenniebrennie

Same here re: findWhere

brenniebrennie

Doc comments should be of the format: /** * Single line summary. * * Multi-line description. */

brenniebrennie

Blank line between these.

brenniebrennie

Undo this formatting change.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

This is unnecessary.

brenniebrennie

This should be a full sentence.

brenniebrennie

"Attributes" should be capitalized here.

daviddavid

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

daviddavid

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);

daviddavid

Instead of !!, can we just compare > 0?

daviddavid

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

daviddavid

I think it would be better to have multiple const reviewReplyDrafts = ... in each place that it's used. Right …

daviddavid

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

daviddavid

There's a better way to wrap this: this.listenTo(reviewReply, 'saved', () => { reviewReplyDrafts = this.get('reviewReplyDrafts'); ... }); (saves one level …

daviddavid

Same wrapping here as above.

daviddavid

Leftover debug output?

daviddavid

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

daviddavid

Probably better to use this.get('reviewReplyDrafts').forEach(reviewReply => { ... }); We're slowly replacing a lot of underscore use with modern JS …

daviddavid

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

daviddavid

Please put a blank line between these two lines.

daviddavid

This is nested very deep. Can we break this stuff out into its own function, and then maybe define the …

daviddavid

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

daviddavid

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

daviddavid

Only one blank line here.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Add this line back please.

daviddavid

Add a blank line between these two.

daviddavid

This can just be: this.listenTo(view, 'fieldSaved', this.showBanner);

daviddavid

Add a blank line between these two.

daviddavid

Better to use listenTo: this.listenTo(this.model, 'saved', this.showBanner);

daviddavid

Alignment is wacky here. Should look like: ```javascript } else if (state === RB.ReviewRequest.PENDING && this.model.get('hasDraft')) { BannerClass = DraftBannerView; …

daviddavid

Should probably save the value inside beforeEach

daviddavid

Should save the value inside beforeEach

daviddavid

Unless we change this, it should be defined as const. This is also a great place to use dedent.

daviddavid

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

daviddavid

Should have a "Returns" section.

daviddavid

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

daviddavid

Formatting of this comment isn't correct. It should be one line summary, blank line, longer description, and then include an …

daviddavid

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

daviddavid

Please include an "Option Args" section.

daviddavid

Dedent this 4 spaces.

daviddavid

Can we line everything up after the (?

daviddavid

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

daviddavid

Dedent this 4 spaces.

daviddavid

"Attributes" should be capitalized.

daviddavid

Type can be Backbone.View or string

daviddavid

Add a blank line between these.

daviddavid

Should just be jQuery for the type.

daviddavid

Please include a doc comment here.

daviddavid

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

daviddavid

"Attributes" should be capitalized.

daviddavid

Indentation in these should be 4 spaces instead of 2.

daviddavid

Needs a "Returns" section.

daviddavid

This can use an ES6 template string: console.error(`Index (${index}) is out of range.`);

daviddavid

Should have a summary/description format. You can probably just add a blank line after "Register an item."

daviddavid

Indentation in these should be 4 spaces instead of 2.

daviddavid

item isn't used outside of the loop, so I'd rather do: while (registryCollection.length > 0) { const item = registryCollection.pop(); …

daviddavid

Probably better to use forEach instead of _.each.

daviddavid

Description should be on the next line after the type.

daviddavid

Indentation should be 4 spaces, not 2.

daviddavid

Should use summary/description format.

daviddavid

Please add a doc comment here.

daviddavid

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

daviddavid

Trailing whitespace

bolariinwabolariinwa

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

SudoliciousSudolicious

I remember saying that we should use this.$el.append

bolariinwabolariinwa
brennie
  1. 
      
  2. Show all issues

    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. Show all issues

    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. Show all issues

    Nit: space between if and (

  5. Show all issues

    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. Show all issues

    ES6 lets us do render(). Same below.

  7. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

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

  4. Show all issues

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

  5. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

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

  5. Show all issues

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

  6. Show all issues

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

  7. 
      
shoven
brennie
  1. 
      
  2. Show all issues

    Can you add screenshots?

  3. Show all issues

    This should be // TODO: Full sentence..

    Though I don't know that this is necessary.

  4. Show all issues

    Same here.

  5. Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Should be organized like:

    .selector {
        property: value;
    
        &-selectors {}
    
        .other-selectors {}
    }
    
  7. Show all issues

    .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. Show all issues

    .selector * is super expensive. See other comment.

  9. Show all issues

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

  10. Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

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

    defaults() {
        return {
            hasDraft: false,
            // ...
        };
    },
    
  12. Show all issues

    Missing doc comment.

  13. Show all issues

    Why these? These are in the defaults.

  14. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    The arrow function should be aligned with the first argument.

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

  15. Show all issues

    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)
    
  16. Show all issues

    We are declaring a variable and then immediately using it once. We should move it inline.

  17. Show all issues

    Doc comments should be of the format:

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

    These arrow functions can be a single line.

  19. Show all issues

    Missing a summary.

  20. Show all issues

    We don't do local vars with leading underscores.

  21. Show all issues

    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 :)

  22. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This should be formatted as either:

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

    (Probably the latter to avoid rightward drift.)

  23. Show all issues

    Blank line between these.

  24. Show all issues

    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.

  25. reviewboard/static/rb/js/models/unifiedReviewBannerState.es6.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This can be rewritten as:

    this.listenTo(
        reviewReply, 'destroying',
        () => this.set('reviewReplyDrafts', _.reject(
            this.get('reviewReplyDrafts'),
            draft => draft.id === reviewReply.id)));
    
  26. Show all issues

    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.

  27. Show all issues

    This should be formatted as:

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

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

  28. Show all issues

    Blank line between these.

  29. Show all issues

    Same here re: findWhere

  30. Show all issues

    Doc comments should be of the format:

    /**
     * Single line summary.
     *
     * Multi-line description.
     */
    
  31. Show all issues

    Blank line between these.

  32. Show all issues

    Undo this formatting change.

  33. Show all issues

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

  34. Show all issues

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

  35. Show all issues

    This is unnecessary.

  36. 
      
brennie
  1. 
      
  2. Show all issues

    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. Show all issues

    "Attributes" should be capitalized here.

  3. Show all issues

    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)
     
     
     
     
     
     
     
     
     
    Show all issues

    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. Show all issues

    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. Show all issues

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

  7. Show all issues

    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. Show all issues

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

  9. Show all issues

    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. Show all issues

    Same wrapping here as above.

  11. Show all issues

    Leftover debug output?

  12. Show all issues

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

  13. Show all issues

    Probably better to use

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

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

  14. Show all issues

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

  15. Show all issues

    Please put a blank line between these two lines.

  16. reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  17. Show all issues

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

  18. Show all issues

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

  19. Show all issues

    Only one blank line here.

  20. Show all issues

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

  21. Show all issues

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

  22. Show all issues

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

  23. Show all issues

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

  24. Show all issues

    Add this line back please.

  25. Show all issues

    Add a blank line between these two.

  26. Show all issues

    This can just be:

    this.listenTo(view, 'fieldSaved', this.showBanner);
    
  27. Show all issues

    Add a blank line between these two.

  28. Show all issues

    Better to use listenTo:

    this.listenTo(this.model, 'saved', this.showBanner);
    
  29. Show all issues

    Alignment is wacky here. Should look like:

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

  30. Show all issues

    Should probably save the value inside beforeEach

  31. Show all issues

    Should save the value inside beforeEach

  32. Show all issues

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

    This is also a great place to use dedent.

  33. Show all issues

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

  34. Show all issues

    Should have a "Returns" section.

  35. Show all issues

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

  36. Show all issues

    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.

  37. Show all issues

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

  38. Show all issues

    Please include an "Option Args" section.

  39. Show all issues

    Dedent this 4 spaces.

  40. Show all issues

    Can we line everything up after the (?

  41. Show all issues

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

  42. Show all issues

    Dedent this 4 spaces.

  43. Show all issues

    "Attributes" should be capitalized.

  44. Show all issues

    Type can be Backbone.View or string

  45. Show all issues

    Add a blank line between these.

  46. Show all issues

    Should just be jQuery for the type.

  47. Show all issues

    Please include a doc comment here.

  48. Show all issues

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

  49. Show all issues

    "Attributes" should be capitalized.

  50. Show all issues

    Indentation in these should be 4 spaces instead of 2.

  51. Show all issues

    Needs a "Returns" section.

  52. Show all issues

    This can use an ES6 template string:

    console.error(`Index (${index}) is out of range.`);
    
  53. Show all issues

    Should have a summary/description format. You can probably just add a blank line after "Register an item."

  54. Show all issues

    Indentation in these should be 4 spaces instead of 2.

  55. Show all issues

    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();
    }
    
  56. Show all issues

    Probably better to use forEach instead of _.each.

  57. Show all issues

    Description should be on the next line after the type.

  58. Show all issues

    Indentation should be 4 spaces, not 2.

  59. Show all issues

    Should use summary/description format.

  60. Show all issues

    Please add a doc comment here.

  61. Show all issues

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

  62. 
      
shoven
bolariinwa
  1. 
      
  2. Show all issues

    Trailing whitespace

  3. 
      
Sudolicious
  1. 
      
  2. Show all issues

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

  3. 
      
shoven
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. Show all issues

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

    1. For now, I'm just appending it to settings_local.py to enable testing while it's still under development and shouldn't be the baseline so I think it's fine for the moment.

  3. 
      
bolariinwa
  1. 
      
  2. Show all issues

    I remember saying that we should use this.$el.append

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

    2. I went back and found the comment in your review request where Barret suggested this. In this case I'm not sure if it makes a difference one way or the other because it will only be initialized once so there's no risk of html wiping anything out by being called again. Maybe Barret can chime in to explain when it would be better to use one over the other or if they have a preference for one over the other in the codebase in general?

    3. Chatted with Barret and he said either way is fine 👍

  3. 
      
shoven
shoven
shoven
david
Review request changed
Status:
Discarded