fix(compass-global-writes): show error on all invalid statuses COMPASS-8451 #6472
Conversation
| > | ||
| <Icon | ||
| glyph="ImportantWithCircle" | ||
| glyph={showWarning ? 'Warning' : 'ImportantWithCircle'} |
There was a problem hiding this comment.
The other way around :) I know it's confusing, but we want it to match the banner icon
| <Icon | ||
| glyph="ImportantWithCircle" | ||
| glyph={showWarning ? 'Warning' : 'ImportantWithCircle'} | ||
| aria-label="warning" |
There was a problem hiding this comment.
Could we make this message and the color of the icon variable as well?
There was a problem hiding this comment.
Is this what you mean?
Error:
Warning:
There was a problem hiding this comment.
Code quality question, it seems weird that the properties of the Icon are so coupled to each other, should we just pull out a WarningIcon and ErrorIcon or something pre-parameterized? Does this pattern come up a lot in compass?
There was a problem hiding this comment.
You're right, with this many differences it makes sense to move the icons into their own function components so that the main render is more readable. And yes that's a common pattern, not only in compass. If your question is more towards reusable components in Compass, check the compass-components package. But I wouldn't put these in there as the warning = ImportantWithcircle and error = Warning is a bit odd. I'd just keep them in the same file.
|
Reviewed with Paula this morning - we think this is LGTM once Paula's refactor PR is in. And that PR makes this one easier to merge so it's better to wait. |
paula-stacho
left a comment
paula-stacho
left a comment
There was a problem hiding this comment.
The last detail - the colors are the other way around 😅 Otherwise looks great!
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes