-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: show fail icon for errors #9272
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code-wise! defer to someone else on if the icon is what we want :)
@@ -705,11 +703,6 @@ | |||
color: var(--color-fail-secondary); | |||
} | |||
|
|||
/* Hide icon if there was an error */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old selves seemed fairly convinced we should intentionally, but why 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5077 seems to be the last time it was touched, so it's fair to say a lot has changed since then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be the last time it was touched, so it's fair to say a lot has changed since then :)
It's a shame we didn't preserve much context. You were very excited @patrickhulce :)
One issue is conflation of lighthouse errors with audit failures, which is something some people already get confused about (thinking "Errror!" means their page had an error). I don't know what a better icon would be, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, yeah it's hard to tell which subchange specifically I was most excited about there, but I remember being super jazzed about the error tooltip and no more informative scores that people have no idea what they mean, so I'm guessing it's somewhat tangential to our change here?
agreed more context would've been helpful now. I also seems worth it to try to move these two states to be less similar in the UI given how frequently people confuse the Error
for their page doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
black hexagon (stop sign)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exterkamp "upside down red triangle. or an X"
@yuinchien what icon should we use for audits with errors? |
or should we be rendering it as normal with an icon at all
this is a really good question. Is there more we can do to show that
|
strikethrough the entire row? |
@paulirish @connorjclark @brendankenny Question 2: the ? in the score seems to be the result of non applicable score. I wonder, if we can try emojis, and show more characters ^_^/ (Just some fun ideas) If you must have an error icon, try this one: https://material.io/tools/icons/?search=error&icon=error&style=baseline |
Just to solve this immediate issue, we'll go with the MD icon. other UI re: errors will probably be handled (heh) later on. |
Updated after image. The icon looks really bad at the regular score icon size, so I doubled it. This required some shifting to keep things lined up. I think the workaround isn't terrible (removed margin and translate a bit to the left). |
@@ -143,9 +143,8 @@ | |||
--plugin-icon-url-dark: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24px" height="24px" viewBox="0 0 24 24" fill="%23FFFFFF"><path d="M0 0h24v24H0z" fill="none"/><path d="M20.5 11H19V7c0-1.1-.9-2-2-2h-4V3.5C13 2.12 11.88 1 10.5 1S8 2.12 8 3.5V5H4c-1.1 0-1.99.9-1.99 2v3.8H3.5c1.49 0 2.7 1.21 2.7 2.7s-1.21 2.7-2.7 2.7H2V20c0 1.1.9 2 2 2h3.8v-1.5c0-1.49 1.21-2.7 2.7-2.7 1.49 0 2.7 1.21 2.7 2.7V22H17c1.1 0 2-.9 2-2v-4h1.5c1.38 0 2.5-1.12 2.5-2.5S21.88 11 20.5 11z"/></svg>'); | |||
--plugin-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="24px" height="24px" viewBox="0 0 24 24" fill="%23757575"><path d="M0 0h24v24H0z" fill="none"/><path d="M20.5 11H19V7c0-1.1-.9-2-2-2h-4V3.5C13 2.12 11.88 1 10.5 1S8 2.12 8 3.5V5H4c-1.1 0-1.99.9-1.99 2v3.8H3.5c1.49 0 2.7 1.21 2.7 2.7s-1.21 2.7-2.7 2.7H2V20c0 1.1.9 2 2 2h3.8v-1.5c0-1.49 1.21-2.7 2.7-2.7 1.49 0 2.7 1.21 2.7 2.7V22H17c1.1 0 2-.9 2-2v-4h1.5c1.38 0 2.5-1.12 2.5-2.5S21.88 11 20.5 11z"/></svg>'); | |||
|
|||
--pass-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>check</title><path fill="%23178239" d="M24 4C12.95 4 4 12.95 4 24c0 11.04 8.95 20 20 20 11.04 0 20-8.96 20-20 0-11.05-8.96-20-20-20zm-4 30L10 24l2.83-2.83L20 28.34l15.17-15.17L38 16 20 34z"/></svg>'); | |||
--average-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>info</title><path fill="%23E67700" d="M24 4C12.95 4 4 12.95 4 24s8.95 20 20 20 20-8.95 20-20S35.05 4 24 4zm2 30h-4V22h4v12zm0-16h-4v-4h4v4z"/></svg>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we weren't using these.
I'm no designer so take with grain of salt but 2x feels like a smidge too far, a few px smaller perhaps 1.5? |
height: calc(var(--score-icon-size) * 2); | ||
transform: translateX(-6px); | ||
margin-right: 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2x does seem too big. 1.7, 1.75?
Also, what do you think about using scaling instead of width/height to avoid the margin shenanigans. Like
.lh-audit--error .lh-audit__score-icon {
background-image: var(--error-icon-url);
transform: scale(1.7);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendankenny is it possible to share a screenshot of the current icon size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one issue is that the scaling doesn't do great things to rendering of the !
. It gets pretty muddled at less than a ~1.7x scale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this mock, i have more context for what you are trying to do.
The original Red triangle already signifies "Error", and doesn't need to be changed. Replacing it with the Material icon feels out of place and creates more confusion for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Red triangle already signifies "Error", and doesn't need to be changed. Replacing it with the Material icon feels out of place and creates more confusion for users.
But today it's not an error, it signifies the page did poorly at something. A really bad FCP is very different than "something went wrong and LH has no idea what your FCP was".
One example of the confusion that sometimes pops up is in #8287
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendankenny my understanding of Runtime Error is "something went wrong and LH doesn't know how to properly produce audit result/score". Maybe we should come up with a more clear label w/o having the word error in it? Something like "Audit invalid", or "Audit unavailable", or "Invalid result" ?
As for the big score with ? mark, might be good to have a info tooltip briefly explain why the score is unavailable.
after today's call, we have a new plan for this: the "thick bang" icon. |
Please see mocks in #9820 |
Are we still good with the thick bang mock? |
related #9968 (comment) seems like this PR addresses one of those two AIs, I'm cool with the exclamation 👍 |
Before:
See #9270
After: