Skip to content
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

[ResponseOps][UI] - EUI Visual Refresh integration and QA #204352

Merged
merged 38 commits into from
Jan 15, 2025

Conversation

georgianaonoleata1904
Copy link
Contributor

@georgianaonoleata1904 georgianaonoleata1904 commented Dec 16, 2024

Summary

Closes: #202551
Closes: #202550
Closes: #202549
Closes: #202548

Meta: #202547

Summary

  • Buttons with success color have been updated to primary
  • Replaced "textSuccess" to "successText"
    example: Edit connector -> Save button
Screenshot 2024-12-17 at 15 32 14
  • 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:
Screenshot 2024-12-18 at 11 50 51

Borealis:
Screenshot 2024-12-18 at 11 50 06

…203068)

Closes #202548
Closes #202549

## Summary

- no changes were needed to the use of "success" color as it was used
for semantic purpose
- replaced "textSuccess" to "successText"

---------

Co-authored-by: Elastic Machine <[email protected]>
…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 <[email protected]>
@cnasikas cnasikas added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0 EUI Visual Refresh labels Dec 16, 2024
packages/kbn-grouping/src/components/grouping.tsx Outdated Show resolved Hide resolved
packages/kbn-grouping/src/components/grouping.tsx Outdated Show resolved Hide resolved
const { euiTheme } = useEuiTheme();

const severityData = {
low: { color: euiTheme.colors.vis.euiColorVis0, label: LOW },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vis colors have changed and don't directly map to similar colors (related conversation)

image

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 #FFC9C2 it's too light for a High Status
  • We use colors.success, colors.warning and colors.danger in 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 😊

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot 2024-12-17 at 13 00 30

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cnasikas The idea is to keep Amsterdam as it is right now and just change the Borealis for the new ones.

Copy link
Member

@cnasikas cnasikas Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Is this a final decision? Could we all agree on this before @georgianaonoleata1904 does the implementation?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgadewoll can you please confirm if you're ok with these changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

color: isActive ? '#a8376a' : euiTheme.colors.subduedText,
color: isActive
? euiTheme.colors.textAccent
: euiTheme.colors.textSubdued,
backgroundColor: isActive ? 'rgba(240,78,152,0.2)' : euiTheme.colors.body,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgadewoll Should we also replace rgba(240,78,152,0.2)?

Copy link
Contributor

@mgadewoll mgadewoll Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed it with @georgianaonoleata1904 and it looks good to me in Amsterdam and Borealis 👍

Copy link
Contributor

@andreadelrio andreadelrio Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good point @andreadelrio, I didn't look into the code, I just did a visual review.
Something to look into @georgianaonoleata1904 @cnasikas.

@georgianaonoleata1904 georgianaonoleata1904 marked this pull request as ready for review December 17, 2024 13:36
@georgianaonoleata1904 georgianaonoleata1904 requested a review from a team as a code owner December 17, 2024 13:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Changes LGTM from EUI side

@joana-cps
Copy link

Visual check done, changes LGTM ✅


const severityData = {
low: {
color: euiTheme.flags.hasVisColorAdjustment ? '#54B399' : '#CAD3E2',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be updated with #204352 (comment) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I hadn't seen the comment above. I'll update, thanks!

interface Props {
selectedSeverity: CaseSeverity;
onSeverityChange: (status: CaseSeverity) => void;
}

export const SeverityFilter: React.FC<Props> = ({ selectedSeverity, onSeverityChange }) => {
const { euiTheme } = useEuiTheme();

const severities = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
alerting 99.0KB 98.7KB -235.0B
cases 538.8KB 539.0KB +249.0B
cloudSecurityPosture 520.7KB 520.7KB +8.0B
observability 480.8KB 480.8KB -30.0B
securitySolution 21.2MB 21.2MB -635.0B
stackConnectors 740.1KB 740.3KB +183.0B
triggersActionsUi 1.7MB 1.7MB +78.0B
total -382.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 25.6KB 25.5KB -54.0B
cases 162.1KB 162.0KB -54.0B
stackConnectors 61.9KB 61.8KB -54.0B
triggersActionsUi 130.2KB 130.2KB +26.0B
total -136.0B

History

cc @georgianaonoleata1904

@georgianaonoleata1904 georgianaonoleata1904 merged commit 10cc891 into main Jan 15, 2025
8 checks passed
@georgianaonoleata1904 georgianaonoleata1904 deleted the ro_eui_theme_borealis branch January 15, 2025 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0
Projects
None yet
10 participants