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

report: metric filter refactor to JS and adornments #12477

Merged
merged 13 commits into from
May 17, 2021
Merged

report: metric filter refactor to JS and adornments #12477

merged 13 commits into from
May 17, 2021

Conversation

paulirish
Copy link
Member

some (potential) followups from #11732

JS refactor: After connor raised the question, I tried out the JS approach to see. I'm fine with either, so if folks prefer the JS style, wfm.

adornments idea: the data side of it was annoying enough to split into this. The UI bit seems interesting, no strong feelings, tbh.

demo: https://lighthouse-git-adorn-googlechrome.vercel.app/english/

@connorjclark
Copy link
Collaborator

connorjclark commented May 14, 2021

In both demo reports, selecting "CLS" removes any relevant audits from the "Passed Audits" section. Ideally that entire section would be hidden while in this state. Same for "Opportunities". Can that be done with just CSS? Probably, because it's not just CSS, you're dynamically writing whatever CSS you want with JS.

For this PR's demo, something weird is happening when you toggle to CLS and then back to All–there is then nothing left in "Passed Audits".

I still think that JS is simpler to understand than dynamically generated CSS.

@paulirish
Copy link
Member Author

selecting "CLS" removes any relevant audits from the "Passed Audits" section. Ideally that entire section would be hidden while in this state.

yeah i was skipping that functionality when this was CSS only. now that it's JS-driven i can add it.

For this PR's demo, something weird is happening when you toggle to CLS and then back to All–there is then nothing left in "Passed Audits".

good catch. i'll fix that.

Base automatically changed from mapping to master May 17, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants