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

[Visual Refresh] Update vis colors and palettes #8112

Merged

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Nov 1, 2024

Summary

closes https://github.com/elastic/eui-private/issues/127

Important

This PR merges into a feature branch.

Note

The first commit of this PR is duplicated from this other PR to ensure shared setup. This commit should not be review on this PR but the previous one.

This PR adds theme specific visualization colors and updates eui color palettes to use theme-specific colors.

Changes

  • adds global EuiVisColorStore - stores current theme vis colors on EuiProvider theme change
    • EUI color palettes previously were using static values and are just - non-react - Javascript; they might be used outside of a react context
    • to support current and new theme this global store provides means to add reactivity to the previously static usage
    • provides pub/sub pattern to ensure consumers can update values on theme update, even outside of the react provider context

Screenshot 2024-11-04 at 10 57 22

  • adds new theme vis color tokens

Screenshot 2024-11-06 at 23 02 42

  • updates EUI color palettes to use vis color tokens stored in the EuiVisColorStore instead of static values
  • updates usages of EUI color palettes
  • migrated "Color palettes" docs to EUI+

#unrelated

QA

@mgadewoll mgadewoll requested a review from tkajtoch November 4, 2024 08:13
@mgadewoll mgadewoll marked this pull request as ready for review November 4, 2024 08:13
@mgadewoll mgadewoll requested a review from a team as a code owner November 4, 2024 08:13
export const euiPaletteForTemperature = function (steps: number): EuiPalette {
const cools = colorPalette([...coolArray.slice().reverse(), '#EBEFF5'], 3);
const warms = colorPalette(['#F4F3DB', ...warmArray], 3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ I made an opinionated change here, that affects Amsterdam:
Here for warm colors it's using #F4F3DB while further down here it's using #FBFBDC.

Those two colors are very similar and to make everything more logical when using tokens, I changed those to use the same color.

Screenshot 2024-11-06 at 07 54 52

Screen.Recording.2024-11-06.at.07.57.25.mov

This will affect Kibana where tests check for color values.

Copy link
Member

Choose a reason for hiding this comment

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

I'm cool with this change as long as designers don't mind. We can merge it to the feature branch and confirm with them

const greenColor: HEX = '#209280';
const redColor: HEX = '#CC5642';
const lightRedColor: HEX = euiPaletteColorBlind()[9];
const coolArray: HEX[] = [euiPaletteColorBlind()[1], '#6092C0'];
Copy link
Contributor Author

@mgadewoll mgadewoll Nov 6, 2024

Choose a reason for hiding this comment

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

ℹ️ This seemed like a mistake, as the coolArray holds the same colors this way because euiPaletteColorBlind()[1] is #6092C0

(2) ['#6092C0', '#6092C0']

};
}

export class EuiVisColorStore {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a class really. In JS it's usually preferred to have it as a function instead.

Copy link
Contributor Author

@mgadewoll mgadewoll Nov 7, 2024

Choose a reason for hiding this comment

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

ℹ️ We agreed to keep the class here for now as refactoring would cost more time and can still be done separately after.
Additionally, our longer term idea is to change how palettes work in general, moving towards context instead.

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

The changes look solid. I don't have any major objections, just the comments I added above.

@mgadewoll mgadewoll force-pushed the eui-theme/127-vis-color-palette branch from f7db3c3 to 9aeec86 Compare November 7, 2024 14:01
@mgadewoll mgadewoll requested a review from tkajtoch November 7, 2024 14:03
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mgadewoll mgadewoll merged commit f561b46 into elastic:eui-theme/borealis Nov 7, 2024
4 checks passed
@mgadewoll mgadewoll deleted the eui-theme/127-vis-color-palette branch November 8, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants