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

feat(styles): eui Amsterdam theme #1463

Merged
merged 22 commits into from
Mar 3, 2022

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Nov 4, 2021

Summary

Move to eui Amsterdam theme. Major changes include:

  • slight color differences (copied from eui, since in javascript)
  • :focus & :focus-within states are slightly different. (see gifs below)
  • slight dimensional changes

After

Screen Recording 2021-11-04 at 05 11 32 PM

Before

Screen Recording 2021-11-04 at 05 09 11 PM

Details

Chart consumes styles from eui to leverage some variables, mixins, etc. and now that kibana is fully on the new Amsterdam theme I think it's best we fully migrate. This was also causing styles issues related to a different implementation for focus states between the old and the new, which consequently applied both styles. This situation defeats any VRT we have in place.

⚠️ This creates a diff for virtually every VRT screenshot 😭 😔

cc: @elastic/eui-design

Issues

fix #1461

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@nickofthyme nickofthyme added chore :styling Styling related issue :all Applies to all chart types :theme labels Nov 4, 2021
@cchaos
Copy link
Contributor

cchaos commented Nov 5, 2021

👋 Hey @nickofthyme! This is great! I just figured I'd give you a heads up that we will be releasing (another) breaking change soon that will actually make Amsterdam the default theme in EUI. This means the import structures will be a bit different affecting this PR.

You may want to pause this until that release happens so you don't have to do the work twice. I can provide guidance on the changes needed once that happens.

@nickofthyme
Copy link
Collaborator Author

@cchaos Thanks for the heads up. I think the main issue with this change is our VRT and having to check all 900+ diffs. If your breaking change would just mean we have to change some imports but the styles are nearly the same than it shouldn't be an issue. When is this change coming? days, weeks, months?

@cchaos
Copy link
Contributor

cchaos commented Nov 5, 2021

It's in this PR elastic/eui#5121 which is in it's final reviews. So maybe next Tuesday (11/9) or the Tuesday after.

@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Nov 9, 2021

@rshen91 found an issue with the fitting functions incorrectly filling the area around orphan data points. I opened an issue for this #1473. This error is in master and unrelated to these changes.

Screen.Recording.2021-11-09.at.05.25.32.PM.mp4

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM - checked all the vrts thanks for opening/referencing the orphan points. All other screenshots only reflected theme changes

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I might be a bit confused about how your docs work or how you expect consumers to consumer charts with/without EUI, but here are some suggestions:

docs/0-Intro/0-Intro.mdx Outdated Show resolved Hide resolved
packages/charts/src/_eui_imports.scss Outdated Show resolved Hide resolved
storybook/style.scss Outdated Show resolved Hide resolved
@nickofthyme
Copy link
Collaborator Author

@markov00 could you take a look at the style @imports? I want to make sure there are not unneeded imports for consuming and usage within charts for the latest eui changes.

@nickofthyme nickofthyme requested a review from cchaos December 15, 2021 19:38
rshen91 and others added 2 commits February 23, 2022 14:45
The `rotation` configuration is available on the `theme.heatmap.xAxisLabel` theme style to rotate the labels of the X-axis from 0 to 90 degrees. The rotation will shrink the chart vertically( and occasionally also horizontally) depending on the length of the labels.
Few changes to the x-axis labels are also landed in this PR:
- categorical labels overflowing the chart boundaries will play a role in shrinking the chart size.
- the X-axis now shows small ticks, center-aligned for categorical scale and left-aligned for time
- the time axis ticks are aligned to the right of the ticks
- time axis labels instead, will not shrink the chart anymore. The number of labels will be reduced gradually to avoid overlaps and overflows. The right edge of the heatmap will always touch the right edge of the rendering area.

BREAKING CHANGES: `width`, `align`, and `baseline` style properties are removed from the `xAxisLabels` and `yAxisLabels` style of the Heatmap theme.

Co-authored-by: Marco Vettorello <[email protected]>
@nickofthyme
Copy link
Collaborator Author

@markov00 do you think this should be a breaking change?

@markov00
Copy link
Member

markov00 commented Mar 3, 2022

@markov00 do you think this should be a breaking change?

Changing the colors is actually a breaking change even if the change is not too drastic (the colors are a bit more washed and the last 2 colors are inverted)

from
Screenshot 2022-03-03 at 15 04 57
to
Screenshot 2022-03-03 at 15 05 04

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Changes looks goot to me tested locally and all charts looks great (only the streamgraph is a bit too transparent, but we can fix this in a different PR)

@nickofthyme nickofthyme merged commit fea1445 into elastic:master Mar 3, 2022
@nickofthyme nickofthyme deleted the amsterdam-theme branch March 3, 2022 15:27
nickofthyme pushed a commit that referenced this pull request Mar 4, 2022
# [45.0.0](v44.0.0...v45.0.0) (2022-03-04)

### Features

* **styles:** eui Amsterdam theme ([#1463](#1463)) ([fea1445](fea1445))

### BREAKING CHANGES

* **styles:** chart color and style changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types chore :styling Styling related issue :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amsterdam theme support
4 participants