[WIP] Using Unified Review Banner feature flag

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

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

Unified Review Banner: Base Shell

Right now I'm working on using the feature flag to replace the existing banners with the unified banner. I basically have it working to replace it (for draft reviews) and change the colour of the unified banner when it becomes a draft. However, I wasn't sure about the draft banner being shown how it was (a listener on the comment being saved shows the banner) and I wanted to understand how it all fits together better so I am still exploring that to see if there is a way I can improve it.



  • 3
  • 0
  • 2
  • 2
  • 7
Description From Last Updated
You only need & if you want to specialize the parent selector e.g.: .foo { &.bar { // Equivalent to ... brennie brennie
This is going to run when the file is sourced, and so this object might not exist? We may want ... brennie brennie
this is undefined here. The second object passed to Backbone.{...}.extend is applied directly to the created object instead of its ... brennie brennie
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
Review request changed
Loading...