Unified Review Banner Base Shell

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

shoven
Review Board
master
d628caa...
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...

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

    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. 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
Review request changed
Loading...