-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
fix: error alert levels again #17027
fix: error alert levels again #17027
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17027 +/- ##
==========================================
- Coverage 76.93% 76.89% -0.05%
==========================================
Files 1030 1031 +1
Lines 55088 55182 +94
Branches 7480 7505 +25
==========================================
+ Hits 42383 42431 +48
- Misses 12454 12499 +45
- Partials 251 252 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This is verified in our env, so ready for a stamp |
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!
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 with minor nit
{!level || level === 'error' ? ( | ||
<Icons.ErrorSolid iconColor={theme.colors.error.base} /> | ||
{level === 'error' ? ( | ||
<Icons.ErrorSolid iconColor={theme.colors[level].base} /> |
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.
To avoid duplication, could we create const iconColor = theme.colors[level].base
above and then reference that here and below?
<Icons.ErrorSolid | ||
className="icon" | ||
iconColor={theme.colors.error.base} | ||
iconColor={theme.colors[level].base} |
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.
Same here
0cec291
to
3fb15e6
Compare
SUMMARY
Even after these fixes, and apparently because this doesn't repro well in the testenv, there were still issues with my previous fix because we use
level
in the styled components (which i missed the first 2 times here).I'm going to cherrypick this fix into our deploy before merging this PR this time, since i've failed to fix this twice already
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI, deploy to our internal env for repro
ADDITIONAL INFORMATION
to: @rusackas @graceguo-supercat @michael-s-molina