Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DBW: Add helpText to report #695

Merged
merged 5 commits into from
Sep 23, 2016
Merged

DBW: Add helpText to report #695

merged 5 commits into from
Sep 23, 2016

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Sep 23, 2016

R: @paulirish @brendankenny @GoogleChrome/lighthouse

Not worth writing tests at this point. Just need something that's less confusing than the current double negatives for failure results: "Site is not using AppCache: No". This also adds help text for each feature and suggestions (with links) on what to use instead.

Current:

screen shot 2016-09-22 at 6 13 54 pm

Now:

screen shot 2016-09-22 at 6 05 28 pm

Eventually we should add a custom report instead of style overrides.

@paulirish
Copy link
Member

IMO we should just change this in report generator. I think this is superior to what the default report has. (Green "Yes", red "No")

@ebidel
Copy link
Contributor Author

ebidel commented Sep 23, 2016

Happy to do that if I have the 👍. Don't want to spend time on it and then be :(

@paulirish
Copy link
Member

👍 from me and @brendankenny on changing the default reporter to use check and X

@ebidel
Copy link
Contributor Author

ebidel commented Sep 23, 2016

PTAL

screen shot 2016-09-22 at 9 25 28 pm

Handlebars.registerHelper('getItemValue', value => {
if (typeof value === 'boolean') {
return value ? 'Yes' : 'No';
return value ? '✔' : '✘';
Copy link
Member

Choose a reason for hiding this comment

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

whoa, never seen this escape.
don't wanna just pass as a literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotta love up on those unicodes.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

looks great!

@paulirish
Copy link
Member

@ebidel with 2ec6b5a in place, do you need the formatters anymore? You can nuke those, right?

Copy link
Contributor Author

@ebidel ebidel left a comment

Choose a reason for hiding this comment

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

Handlebars.registerHelper('getItemValue', value => {
if (typeof value === 'boolean') {
return value ? 'Yes' : 'No';
return value ? '✔' : '✘';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotta love up on those unicodes.

@ebidel
Copy link
Contributor Author

ebidel commented Sep 23, 2016

Or maybe I don't? Is there a formatter that already does that?

@ebidel
Copy link
Contributor Author

ebidel commented Sep 23, 2016

upstreamed the helpText to metadata and got rid of the dbw custom formatters.

screen shot 2016-09-23 at 9 20 31 am

PTAL

@ebidel ebidel changed the title DBW: Add preliminary formatter DBW: Add helpText to report Sep 23, 2016
@brendankenny
Copy link
Member

I'll take a look at this this morning

Copy link
Member

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

Looks good. Not a fan of yet one more field there, but report needs a good reworking anyways, and this will actually help make clear all the things some future solution will need to support.

@brendankenny
Copy link
Member

sorry, not the morning anymore :)

@brendankenny brendankenny merged commit 95bd2af into master Sep 23, 2016
@brendankenny brendankenny deleted the dbwformatter branch September 23, 2016 22:47
@paulirish
Copy link
Member

This is a great PR. Already loving the new looks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants