STN-39 Site scores teaser #32
Conversation
garrettr
force-pushed
the
site-score-preview
branch
from
b965989
to
98a938b
Compare
garrettr
force-pushed
the
leaderboard-ux
branch
from
2370a4d
to
9f084da
Compare
Adds a Backbone View for the site score teaser on the homepage. It uses the same basic approach as the Leaderboard for instantiating itself with JSON injected into the template. This approach is superior to randomizing the sites on the server, because it improves the cacheability of the homepage (at the cost of increasing the size of the page, but it's not by much).
- Equalize the amount of empty space above and below the site score teaser - Reduce the amount of empty space below the "See Full Leaderboard" button - it's a waste of space and looks awkward.
garrettr
force-pushed
the
site-score-preview
branch
from
98a938b
to
aac6cd6
Compare
| }, | ||
|
|
||
| render() { | ||
| this.$el.fadeOut(500, function () { |
There was a problem hiding this comment.
Use arrow functions instead of bind for anonymous callbacks (arrow functions use the containing closure's value of this instead of rebinding).
| let teaserSites = null; | ||
| let siteNamesLength = null; | ||
| do { | ||
| teaserSites = new Sites(this.collection.sample(TEASER_SAMPLE_SIZE)); |
There was a problem hiding this comment.
I would move the definition of TEASER_SAMPLE_SIZE into this function, since you can't really change it without being aware of how this works. A too large value will cause an infinite loop.
There was a problem hiding this comment.
Also REALLY minor nit, but I would define teaserSites as a const with an empty list of models, and then call reset with the new models instead of allocating a new object each time.
| this.startRandomizingTeaser.bind(this)); | ||
| }, | ||
|
|
||
| render() { |
There was a problem hiding this comment.
Non-idempotent render methods make me a little nervous. It makes a view harder to extend and modify. Obviously this a really small view that likely won't change, but if you wanted to refactor slightly I would suggest the following:
- Move teaserSites out of the state (optionally, but it will make the following easier to type 😄 )
- Refactor randomizeTeaser to update one persistent collection, rather than blowing the old one away.
- Move the fadeIn/out functionality to a separate method. (
rotateTeaser, e.g.) - Call
rotateTeaserforresetevents on the teaserSites collection.
| this.state = new Backbone.Model({ | ||
| teaserSites: new Sites(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
I like to define all of the instance variables that I'm going to use in the constructor, just as documentation, so I would add this.timer = null. Alternatively you could add it to the state if you do the idempotent render refactor I suggest below.
| @@ -20,5 +20,5 @@ section { | |||
| .blue-section { | |||
| color: #fff; | |||
| background: $blueGradient; | |||
| padding-bottom: $spacingUnit * 3; | |||
| padding-bottom: $spacingUnit * 1; | |||
There was a problem hiding this comment.
This class is used on every page, does this change look good everywhere?
There was a problem hiding this comment.
Sorry, I should've added a note about testing this in the PR description. I checked the homepage, leaderboard, content pages, blog index page, and blog post pages and thought this change looked ok on all of them.
I'd like to do a more holistic review of the overall styles before release, but I think that should happen once the rest of the frontend components are fairly stable and unlikely to change.
| @@ -56,6 +58,11 @@ def get_context(self, request): | |||
| context['percent_defaulting_to_https'] = math.floor( | |||
| len(sites_defaulting_to_https) / total_sites * 100) | |||
|
|
|||
| # Serialize the sites with the results of their latest scans for the | |||
| # teaser | |||
| sites = Site.objects.all() | |||
There was a problem hiding this comment.
Django is caching this query for us, right? You could avoid repeating it by moving this above line 45.
| // the site's names is too great, the spans will wrap around within the | ||
| // containing div, which looks weird. This code avoids that problem by | ||
| // re-sampling until a sample that does not trigger the issue is found. | ||
| const maxSiteNamesLength = 50; // Determined empirically |
There was a problem hiding this comment.
What screen width did you test this at?
There was a problem hiding this comment.
My 13" MBP (2560 x 1600 Retina).
There was a problem hiding this comment.
Yeah, this method is kind of a quick-n-dirty way to resolve the (infrequent, but buggy-looking) span wrap problem. I am open to alternative solutions! In the mean time I am in "it works alright, ship it" mindset.
Some reorganization and comment cleanup for clarity.
- idempotent render method - handle display animation in rotateTeaser method - move sites out of state and into view; use collection reset to update - use consistent naming for constants
Adds a Backbone View for the site score teaser on the homepage.
Based on leaderboard-ux because I needed the new logic that computes site grades in the model. I intend to merge this into master separately once leaderboard-ux is merged in #28.
@GabeIsman If you have time, would you review this? I'm mostly interested in your take on the Backbone View and the changes to the SCSS. I think it's pretty straightforward, and doesn't need your review to merge, but as always feedback is appreciated!