-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security team] Update components for EUI visual refresh #201795
[Security team] Update components for EUI visual refresh #201795
Conversation
Hi @elastic/eui-team There are a few places we use the following hard coded colors:
Do you have any recommended color tokens for these? I couldn't find exact matches in the palettes or sass variables. Any suggestions? |
@elasticmachine merge upstream |
@SiddharthMantri Could you provide us with a context of where/for what these colors are used for? And in which color mode are those used, light or dark mode? Because e.g. a light grey might be a background or a text color or something else 🙂 |
@mgadewoll Of course! Adding some context here:
|
@SiddharthMantri Here is the mapping we're recommending based on the usage.
|
@@ -52,7 +52,7 @@ export const MyPluginComponent: React.FC = () => { | |||
).map(([capability, value]) => { | |||
return value === true ? ( | |||
<div key={capability}> | |||
<EuiHealth color="success">{capability}</EuiHealth> | |||
<EuiHealth color="accentSecondary">{capability}</EuiHealth> |
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.
@SiddharthMantri Upon checking with the EUI team, it appears we won't have accentSecondary
as an option for EuiHealth. Without having seen the screenshot for this I imagine this EuiHealth element is trying to convey something positive, so staying with the success
color here would be the recommended course of action.
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.
Thanks @andreadelrio! Fixed in 9ef4111
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, pending update re: Andrea's comment
@elasticmachine merge upstream |
@@ -76,7 +78,7 @@ export const SpaceListInternal = ({ | |||
defaultMessage: `* All spaces`, | |||
}), | |||
initials: '*', | |||
color: '#D3DAE6', | |||
color: euiTheme.colors.vis.euiColorVis3, |
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.
@SiddharthMantri can you please provide us with a screenshot of this? cc @gvnmagni
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.
@andreadelrio Of course! i just added the before and after to the description above.
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.
@SiddharthMantri Thanks, it looks like the color before was this gray though (#D3DAE6), not the purple shown in your screenshot?
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.
@andreadelrio Oops, you're right. I was using the recommendation from here: #201795 (comment) Happy to switch to whichever color is closest.
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.
@SiddharthMantri thanks! I asked Lene a clarifying question regarding this.
@mgadewoll can I ask you why |
@andreadelrio The recommendation was done without me fully knowing what it's used for, I thought it's part of the vis colors range somehow (used for badges and avatars etc). If it's used more generally, then it should not be changed, that's my bad! |
Thanks @mgadewoll! Updated the color and the screenshots. |
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
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.
Style changes LGTM. Thanks!
@elasticmachine merge upstream |
Closes #200005
Summary
Integrate changes from the Borealis theme to components owned by @elastic/kibana-security team.
Notes
There are no visual changes in this PR. However:
success
toaccentSecondary
where neededto
colors.textDisabled`Screenshots
There isn't much to add but adding a few before/after screenshots of the changes made
How to test
Stack Management > Advance Setting
and change the theme to Borealis.Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:*
label is applied per the guidelinesRelease notes
Update Kibana Security components to use new EUI Borealis theme.