Skip to content

Conversation

@connorjclark
Copy link
Collaborator

Fixes #9058

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// Fireworks.
const scoresAll100 = Object.values(report.categories).every(cat => cat.score === 1);
if (!this._dom.isDevTools() && scoresAll100) {
const hasAllCoreCategories =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I guess this would still get fireworks for custom config categories but if the user is going all that way to get some fireworks they probably deserve 'em 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of feel like it should be at least perf + any other categories need to be 100. So solo perf 100 would be ok.

But I also don't care that much and this seems fine :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solo + fireworks would require a refactor, since the fireworks were designed to work with the scores container header.

if (!this._dom.isDevTools() && scoresAll100) {
const hasAllCoreCategories =
Object.keys(report.categories).filter(id => !Util.isPluginCategory(id)).length >= 5;
if (!this._dom.isDevTools() && scoresAll100 && hasAllCoreCategories) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brave soul creating merge conflicts for yourself 😉

Copy link
Contributor

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit and LGTM

/* globals getFilenamePrefix Util */

/** @typedef {import('./dom.js')} DOM */
/** @typedef {import('./util.js')} Util */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the html-renderer.d.ts take care of this? (I don't think either typedef is needed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, removed

// Fireworks.
const scoresAll100 = Object.values(report.categories).every(cat => cat.score === 1);
if (!this._dom.isDevTools() && scoresAll100) {
const hasAllCoreCategories =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of feel like it should be at least perf + any other categories need to be 100. So solo perf 100 would be ok.

But I also don't care that much and this seems fine :)

@connorjclark connorjclark changed the title report: only show fireworks if all core categories are present report: show fireworks only if all core categories score 100 May 30, 2019
@connorjclark connorjclark merged commit f17e6be into master May 30, 2019
@brendankenny brendankenny deleted the firework-all-core branch May 30, 2019 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fireworks don't work for solo category

4 participants