Help text toggle #751
Help text toggle #751
Conversation
| @@ -34,6 +34,23 @@ class ReportGenerator { | |||
| return Math.round(totalScore * 100); | |||
| }; | |||
|
|
|||
| const getItemRating = (value, aggregatorScored) => { | |||
| if (typeof value === 'boolean') { | |||
| return value ? 'good' : 'poor'; | |||
There was a problem hiding this comment.
nit extract good / poor / average to a file live "enum" like map const
|
|
||
| // Limit the rating to average if this is a rating for Best Practices. | ||
| let rating = aggregatorScored ? 'average' : 'poor'; | ||
| if (value > 0.33) { |
There was a problem hiding this comment.
extract magic numbers to a private file level const map
There was a problem hiding this comment.
not going to touch other people's stuff as part of this cl
There was a problem hiding this comment.
I agree with sam here. If you can improve code immediately involved with what you're changing anyways, you should. Pulling these out would be helpful.
There was a problem hiding this comment.
If every PR is a refactor, PRs take way longer to get in and introduce errors unrelated to the PR. I'm happy to do this one because it's small.
There was a problem hiding this comment.
Agreed, and there's always a limit so we can actually make progress, PRs aren't weighted down by the past, etc. But small simplifications along the way are also a good way to help pay down technical debt. People usually need to get new stuff done so pure refactor PRs don't come along that often.
| @@ -47,9 +49,19 @@ LighthouseReport.prototype = { | |||
| } | |||
| }, | |||
|
|
|||
| toggleHelpText: function(e) { | |||
| if (e.target.classList.contains('report-section__item-help-toggle')) { | |||
| const el = e.currentTarget.parentNode.querySelector('.report-section__item-helptext'); | |||
There was a problem hiding this comment.
depending directly on the parentNode structure feels like smell, and potentially brittle code that could break with a trivial dom change.
There was a problem hiding this comment.
Moved the handlers up a level to each item in the report (.report-section__item). That should "guarantee" the help text falls within the currentTarget
| addEventListeners: function() { | ||
| this.printButton.addEventListener('click', this.onPrint); | ||
| this.checkboxToggleView.addEventListener('change', this.updateView); | ||
| Array.from(this.itemDetails).forEach(node => { |
There was a problem hiding this comment.
[...this.itemDetails].forEach
should be fine, since itemDetails is an iterable
|
PTAL |
|
"1 resources were not served over h2"?? |
|
bugs me too :) |
| @@ -34,6 +40,23 @@ class ReportGenerator { | |||
| return Math.round(totalScore * 100); | |||
| }; | |||
|
|
|||
| const getItemRating = (value, aggregatorScored) => { | |||
There was a problem hiding this comment.
make this a method on the class?
There was a problem hiding this comment.
aggregatorScored appears to never be used
There was a problem hiding this comment.
It's an arg to the helper, but I don't see it used in the report.
There was a problem hiding this comment.
what about moving this to a private method?
|
PTAL |
|
travis is apparently busy taking a nap |
|
This PR is the bee's knees. 👍 |
R: @paullewis @paulirish @brendankenny
helpTextby default when the audit rating is "good". Shows it otherwise.helpText, which is why nothing is showing for that failed audit :)