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

[Meta] Reclaim chart theme from eui #2069

Closed
5 tasks done
nickofthyme opened this issue Jun 12, 2023 · 9 comments
Closed
5 tasks done

[Meta] Reclaim chart theme from eui #2069

nickofthyme opened this issue Jun 12, 2023 · 9 comments
Assignees
Labels
enhancement New feature or request meta ...meta issue :theme

Comments

@nickofthyme
Copy link
Collaborator

nickofthyme commented Jun 12, 2023

Per discussions from #865, we have determined to go ahead with moving the charts theme from eui back to charts.

Action items for EUI

Font family

Resolve which font family to use for charts theme? Currently the eui charts theme
defines the fontFamily here as...

const fontFamily = `'Inter', 'Inter UI', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol'`;

but the eui theme JSON defines three font families as...

{
  ...
  "font": {
    "family": "'Inter', BlinkMacSystemFont, Helvetica, Arial, sans-serif",
    "familyCode": "'Roboto Mono', Menlo, Courier, monospace",
    "familySerif": "Georgia, Times, Times New Roman, serif",
    ...
    }
 }

I assume for charts we want to use theme.font.family, needs confirmation.

Export colors

Export computed theme colors here to either the theme JSON or a separate colors-only JSON distributable for dark and light themes.

Action items for charts

Update existing charts theme

Update the charts theme to conform to the current eui chart theme defaults found here. This includes importing the colors and fontFamily from eui JSON and/or palette functions. Also need to append the colors with chartLines and chartBand defined per theme here.

Final logistical steps

  • Release charts version with new themes.
  • Update charts in kibana.
    • Verify changes to theme with all stakeholders in kibana.
    • Remove eui chart theme logic. Could simply replace theming first and simplify chart theme service later.
  • Remove chart theming from eui, update eui in kibana. chore(charts): remove eui chart theme eui#7572
  • Update PR template action referring to theme changes.
  • Update storybook to test only new themes and remove logic for eui theme support.

Future tasks

Handle dynamic theme changes

Possibly by creating a global charts provider in kibana to update theming based on changes to EuiProvider base theming.

Migrate away from eui sass

Charts currently relies on several sass files from eui (see _eui_imports.scss) to match basic styles from eui. These include variables, functions and mixins.

Need to discuss how to resolve this - possible options are:

  • Duplicate functionality in charts.
  • Use custom chart styles to get close.
  • Somehow use eui emotion styles in charts (Migrate chart styles to pure javascript).
@stratoula
Copy link

@nickofthyme can we close this?

@markov00
Copy link
Member

not yet, there are a few steps more to complete this

@stratoula
Copy link

@markov00 cool, do we want to work on them on 8.13?

@markov00
Copy link
Member

@stratoula sounds fine for me

@nickofthyme
Copy link
Collaborator Author

Sorry @stratoula,

Yes after merging elastic/kibana#170914 we can close this. All we need to do after is remove the chart theme logic from eui and eventually remove the deprecated themes from charts down the road.

@markov00
Copy link
Member

@nickofthyme can we complete this before going on with other stuff on 8.14?

@nickofthyme
Copy link
Collaborator Author

Yup will do!

@nickofthyme
Copy link
Collaborator Author

Closing with merging elastic/eui#7572

@nickofthyme
Copy link
Collaborator Author

Finally removing all theme export from eui in elastic/eui#7682. Finishes this issue in full.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request meta ...meta issue :theme
Projects
None yet
Development

No branches or pull requests

3 participants