[go: nahoru, domu]

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: update error icons #15092

Merged
merged 11 commits into from
May 22, 2023

Conversation

tannerdolby
Copy link
Contributor
@tannerdolby tannerdolby commented May 18, 2023

Summary

Fixes #9820

Updates runtime error icons to use the SVGs provided in #9820 (comment)

Screen Shot 2023-05-17 at 11 30 28 PM

Testing

  • run yarn build-sample-reports
  • view the .cdt.error/index.html or .psi.error/index.html sample reports
  • see the updated runtime error icon

Related Issues/PRs

#9272
#9968 (comment)

@tannerdolby tannerdolby requested a review from a team as a code owner May 18, 2023 06:35
@tannerdolby tannerdolby requested review from connorjclark and removed request for a team May 18, 2023 06:35
@adamraine
Copy link
Member

Looks good this is a great start! It would be nice to have a single definition for this svg rather than four different ones (3 sizes in CSS, and 1 created in JS).

Also, noticed the icon was slightly off center in audit lists:

Screenshot 2023-05-18 at 10 37 47 AM

report/assets/styles.css Outdated Show resolved Hide resolved
@tannerdolby
Copy link
Contributor Author
tannerdolby commented May 19, 2023

@adamraine Thanks for the review! I've updated the svg definitions and now there is only one definition in CSS. I did some tinkering and found a way to get the positioning right using only background-images.

I think the icon being off center in the audit list was partly due to the svg definition I originally had, it should be updated now and appear vertically centered in the row on Chrome (Version 113.0.5672.126) and Firefox (109.0.1). Let me know if it still appears off and I'll try to reproduce the alignment issue to fix it.

Screen Shot 2023-05-18 at 8 12 06 PM

report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
Copy link
Member
@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

This looks great! I made some changes to scale up the svg to fit it's view box, then scale it down by 50% when it's in a score gauge. This allows us to inherit a lot of the same CSS used to render the normal score icons in audits and metrics.

@tannerdolby
Copy link
Contributor Author

Awesome! I just checked out your changes and everything looks great. That was a smart idea to scale the svg to allow for us to inherit a lot of the same CSS.

@adamraine adamraine merged commit 0123234 into GoogleChrome:main May 22, 2023
33 checks passed
@tannerdolby tannerdolby deleted the update-runtime-error-icon branch May 22, 2023 19:14
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.

Runtime Error Design Update
4 participants