[ResponseOps][UI] - EUI Visual Refresh integration and QA#204352
[ResponseOps][UI] - EUI Visual Refresh integration and QA#204352georgianaonoleata1904 merged 38 commits intomainfrom
Conversation
…precated color variables (#203120) Closes #202550 ## Summary - All references to renamed tokens have been updated to use the new token name # previous color token -> new color token primaryText -> textPrimary accentText -> textAccent between this and textSuccess. warningText -> textWarning dangerText -> textDanger text -> textParagraph title -> textHeading subduedText -> textSubdued disabledText -> textDisabled successText -> textSuccess - replaced the color utility functions with EUI color tokens as they will be deprecated
…ate with theme changes (#203448) Closes #202551 ## Summary - All usage of color palette tokens and functions now pull from the theme JSON tokens are anything from: @elastic/eui/dist/eui_theme_light.json @elastic/eui/dist/eui_theme_dark.json import { euiThemeVars } from '@kbn/ui-theme' --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
packages/response-ops/rule_form/src/rule_actions/rule_actions_item.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/alerting/public/pages/maintenance_windows/components/page_header.tsx
Outdated
Show resolved
Hide resolved
| const { euiTheme } = useEuiTheme(); | ||
|
|
||
| const severityData = { | ||
| low: { color: euiTheme.colors.vis.euiColorVis0, label: LOW }, |
There was a problem hiding this comment.
The vis colors have changed and don't directly map to similar colors (related conversation)
Since these are used as severity colors here, you likely want to ensure they are still using a specific color.
The severity color palette is still being worked on, but we can remap vis colors per theme.
I'm not sure who's the responsible designer to involve here for this code? cc @gvnmagni
We have two options here:
- use new tokens which mean the current Amsterdam colors change a bit (the main issue seems to be distinguishing high and critical)
- map severity colors per theme
Option 1
low: { color: euiTheme.colors.vis.euiColorVisSuccess0, label: LOW }
medium: { euiTheme.colors.vis.euiColorVisWarning0, label: MEDIUM }
high: { color: euiTheme.colors.vis.euiColorVisDanger1, label: CRITICAL }
critical: { color: euiTheme.colors.vis.euiColorVisDanger0, label: CRITICAL }| Severity level | previous value | Amsterdam | Borealis |
|---|---|---|---|
| low | #54B399 |
#209280 |
#24C292 |
| medium | #D6BF57 |
#D6BF57 |
#FCD883 |
| high | #DA8B45 |
#E7664C |
#FFC9C2 |
| critical | #E7664C |
#CC5642 |
#F6726A |
Option 2
const { euiTheme } = useEuiTheme();
const { hasVisColorAdjustment } = euiTheme;
low: { color: hasVisColorAdjustment ? euiTheme.colors.vis.euiColorVis0 : euiTheme.colors.vis.euiColorVisSuccess0, label: LOW }
medium: { color: hasVisColorAdjustment ? euiTheme.colors.vis.euiColorVis5 : euiTheme.colors.vis.euiColorVisWarning0, label: MEDIUM }
high: { color: hasVisColorAdjustment ? euiTheme.colors.vis.euiColorVis7 : euiTheme.colors.vis.euiColorVisDanger1, label: CRITICAL }
critical: { color: hasVisColorAdjustment ? euiTheme.colors.vis.euiColorVis9 : euiTheme.colors.vis.euiColorVisDanger0, label: CRITICAL }| Severity level | previous value | Amsterdam | Borealis |
|---|---|---|---|
| low | #54B399 |
#54B399 |
#24C292 |
| medium | #D6BF57 |
#D6BF57 |
#FCD883 |
| high | #DA8B45 |
#DA8B45 |
#FFC9C2 |
| critical | #E7664C |
#E7664C |
#F6726A |
There was a problem hiding this comment.
Hi @mgadewoll, thanks for the detailed suggestion here. @joana-cps is the designer in charge of this area of the product. I'm personally ok with either option. Will defer to @gvnmagni or @joana-cps.
There was a problem hiding this comment.
@joana-cps let's chat about this, I want to be sure I can support you but I need to understand a little bit better the context
There was a problem hiding this comment.
Hi @mgadewoll, thanks very much for the detailed suggestion here, this was a topic that we spent some time discussing with the team (@georgianaonoleata1904 and @cnasikas) and we agreed on using this approach:
| Severity level | previous value | Amsterdam | Borealis | Color token |
|---|---|---|---|---|
| low | #54B399 |
#00BFB3 |
#008A5E |
colors.success |
| medium | #D6BF57 |
#FEC514 |
#FACB3D |
colors.warning |
| high | #DA8B45 |
#F04E98 |
#BC1E70 |
colors.accent |
| critical | #E7664C |
#BD271E |
#C61E25 |
colors.danger |
The main reasons:
- We tried both proposals but in Borealis
#FFC9C2it's too light for a High Status - We use
colors.success,colors.warningandcolors.dangerin other ResponseOps pages, so we would like to have some consistency in our status - We use same color token for both themes
- The new severity colors change will have less impact since we are already using dark red/pink in higher status
We were missing an "orange" for the high status, and this was the best solution that we could find
Let me know what you think 😊
There was a problem hiding this comment.
ok, I got the chance to chat with Joana and I believe we reached a consensus.
From my perspective, I would not change anything in Amsterdam, while for Borealis I would proceed this way. We don't have yet all the tokens set up for our new Severity Color Palette (issue) but what we can do is to apply color primitives directly and eventually come back here introducing severity tokens as soon as they are ready.
Specifically, this would be the mapping of our levels in this case:
There was a problem hiding this comment.
@cnasikas The idea is to keep Amsterdam as it is right now and just change the Borealis for the new ones.
There was a problem hiding this comment.
Ok! Is this a final decision? Could we all agree on this before @georgianaonoleata1904 does the implementation?
There was a problem hiding this comment.
@mgadewoll can you please confirm if you're ok with these changes?
There was a problem hiding this comment.
Yes, I agree. Let's keep Amsterdam as is and use the new ones for Borealis.
Fyi, we're introducing new tokens for severity colors. These will then replace these mappings, but it will be done as a follow-up.
There was a problem hiding this comment.
Fyi, the severity colors are already available on euiTheme.colors.vis
Suggestion based on @gvnmagni comment above:
low: isAmsterdam ? euiTheme.colors.vis.euiColorVis0 : euiTheme.colors.vis.euiColorSeverity5
medium: isAmsterdam ? euiTheme.colors.vis.euiColorVis5 : euiTheme.colors.vis.euiColorSeverity7
high: isAmsterdam ? euiTheme.colors.vis.euiColorVis7 : euiTheme.colors.vis.euiColorSeverity10
critical: isAmsterdam ? euiTheme.colors.vis.euiColorVis9 : euiTheme.colors.vis.euiColorSeverity14
That results in
| Amsterdam | Borealis |
|---|---|
#54B399 |
#CAD3E2 |
#D6BF57 |
#FCD883 |
#DA8B45 |
#FC9188 |
#E7664C |
#C61E25 |
...plugins/triggers_actions_ui/public/application/sections/alerts_page/hooks/use_rule_stats.tsx
Outdated
Show resolved
Hide resolved
| color: isActive | ||
| ? euiTheme.colors.textAccent | ||
| : euiTheme.colors.textSubdued, | ||
| backgroundColor: isActive ? 'rgba(240,78,152,0.2)' : euiTheme.colors.body, |
There was a problem hiding this comment.
@mgadewoll Should we also replace rgba(240,78,152,0.2)?
There was a problem hiding this comment.
Maybe, yes. Does it need the transparency or can it be a solid color?
Just looking at the code, I'd think it can become a solid color as this is a button. And based on button definitions it should then probably use euiTheme.colors.backgroundLightAccent.
Edit: One note on that: This will change the color for current Amsterdam, so I'd advise to check if that change is ok.
There was a problem hiding this comment.
There was a problem hiding this comment.
I reviewed it with @georgianaonoleata1904 and it looks good to me in Amsterdam and Borealis 👍
There was a problem hiding this comment.
@joana-cps Not blocking but looking at the code only, I'm curious why we are using a button here instead of EuiButton and why we're passing a custom color. Something to consider for later.
There was a problem hiding this comment.
yes good point @andreadelrio, I didn't look into the code, I just did a visual review.
Something to look into @georgianaonoleata1904 @cnasikas.
|
Pinging @elastic/response-ops (Team:ResponseOps) |
mgadewoll
left a comment
There was a problem hiding this comment.
✅ Changes LGTM from EUI side
…o ro_eui_theme_borealis
|
Visual check done, changes LGTM ✅ |
…o ro_eui_theme_borealis
|
|
||
| const severityData = { | ||
| low: { | ||
| color: euiTheme.flags.hasVisColorAdjustment ? '#54B399' : '#CAD3E2', |
There was a problem hiding this comment.
should this be updated with #204352 (comment) ?
There was a problem hiding this comment.
You're right, I hadn't seen the comment above. I'll update, thanks!
| export const SeverityFilter: React.FC<Props> = ({ selectedSeverity, onSeverityChange }) => { | ||
| const { euiTheme } = useEuiTheme(); | ||
|
|
||
| const severities = { |
There was a problem hiding this comment.
what do you think about taking the chance to have just one source of truth? We seem to have the same object in x-pack/platform/plugins/shared/cases/public/components/severity/config.tsx
There was a problem hiding this comment.
done, thanks!
There was a problem hiding this comment.
I think you forgot to update here, right? We discussed via dm to not apply this change because of the package being private it can't be exported and we agreed that it would be out of scope to solve it.
…o ro_eui_theme_borealis
…o ro_eui_theme_borealis
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
History
|
…4352) ## Summary Closes: elastic#202551 Closes: elastic#202550 Closes: elastic#202549 Closes: elastic#202548 Meta: elastic#202547 Summary - Buttons with `success` color have been updated to `primary` - Replaced "textSuccess" to "successText" example: Edit connector -> **Save** button <img width="756" alt="Screenshot 2024-12-17 at 15 32 14" src="https://github.com/user-attachments/assets/5d953c7a-6855-4718-98ab-fc89c516ff8d" /> - All references to renamed tokens have been updated to use the new token name - Replaced the color utility functions with EUI color tokens as they will be deprecated - All usage of color palette tokens and functions now pull from the theme - changed severity colors in Borealis Amsterdam: <img width="1931" alt="Screenshot 2024-12-18 at 11 50 51" src="https://github.com/user-attachments/assets/401465b5-b589-47c7-82a6-8211f06c13a5" /> Borealis: <img width="1925" alt="Screenshot 2024-12-18 at 11 50 06" src="https://github.com/user-attachments/assets/47ebcba7-70b0-4875-906a-55f509e0b5ca" /> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

Summary
Closes: #202551
Closes: #202550
Closes: #202549
Closes: #202548
Meta: #202547
Summary
successcolor have been updated toprimaryexample: Edit connector -> Save button
Amsterdam:

Borealis:
