Skip to content

Update elastic charts to 20.0.2#3792

Merged
nickofthyme merged 13 commits intoelastic:masterfrom
nickofthyme:upgrade-charts
Aug 6, 2020
Merged

Update elastic charts to 20.0.2#3792
nickofthyme merged 13 commits intoelastic:masterfrom
nickofthyme:upgrade-charts

Conversation

@nickofthyme
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme commented Jul 23, 2020 •
edited
Loading

Summary

  • update createTheme function with theme changes
  • add @elastic/charts under optionalDependencies

The charts Theme now allows for inner and outer padding of axis title and labels. As well as tick visibility which now removes tick line padding as well.

@cchaos feel free to change any of the styles to your liking.

Before

image

After

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@nickofthyme
Update elastic charts to 20.0.2
b75e6b0 
- update `createTheme` function with theme changes
- add `@elastic/charts` under `optionalDependencies`
@nickofthyme nickofthyme added the dependencies PRs that update a dependency file label Jul 23, 2020
nickofthyme
nickofthyme commented Jul 23, 2020
View reviewed changes
nickofthyme
nickofthyme commented Jul 23, 2020
View reviewed changes
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 23, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 23, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 23, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

chandlerprall
chandlerprall suggested changes Jul 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

@cchaos thoughts on marking this as a breaking change, as the EUI theme configuration would be invalid for previous chart versions?

@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Jul 23, 2020

@cchaos thoughts on marking this as a breaking change, as the EUI theme configuration would be invalid for previous chart versions?

To be clear, these changes would not throw any errors with respect to the chart, but the styling would be different until these changes are added to kibana. I was hoping to upgrade eui and ec together in the same pr on kibana but I noticed eui is a major version ahead and not sure when y'all plan to upgrade eui in kibana to the latest version.

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Jul 23, 2020

All the charts in the examples render correctly to me with these changes. Thank you for tackling our side of it!

I have one question about a rendering problem we have and not sure if they've been fixed in Charts yet, but:

When the axis tick labels are missing, they still take up space
Screen Shot 2020-07-23 at 13 26 00 PM

Screen Shot 2020-07-23 at 13 26 21 PM

As for the "breaking change", what is the industry standard for considering if a boost in dependency version has been changed? I'm ok not considering the actual theme changes a break.

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jul 23, 2020

As for the "breaking change", what is the industry standard for considering if a boost in dependency version has been changed?

Strictly, it's a breaking change. A weird here because the chart theming isn't our main published package. I'd lead toward breaking, tho, as it is more accurate.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jul 23, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Jul 23, 2020 •
edited
Loading

@cchaos yes that is fixed in this version. The new padding layout more customizable. The padding layout is now as follows for a left-side axis and is analogous for other sides (i.e. mirrored or rotated)

Each of the colors are tied together in terms of visibility. So if the label visibility is set to false, the label and all associated padding is removed with it.

image

@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Jul 23, 2020

...So if the tickLine.visible = false and tickLabel.padding.inner = 0, you would get something like this...

image

@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Jul 23, 2020

Sorry I thought you were referring to the tickLine but yes the same applies to the tickLabel

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Aug 3, 2020

Thanks, @nickofthyme. Can you resolve the conflicts on this PR? Then I'll throw you a commit that'll fix the "Sizing" example.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 4, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 4, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 4, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Aug 4, 2020

retest

@cchaos cchaos requested a review from chandlerprall August 4, 2020 20:47
cchaos
cchaos approved these changes Aug 4, 2020
View reviewed changes
Copy link
Copy Markdown
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.

👍 LGTM, though I guess we also need a Breaking change changelog item specifying the upgrade in dependency

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 4, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Aug 5, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3792/

@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Aug 5, 2020

@chandlerprall are you ok with these changes?

chandlerprall
chandlerprall approved these changes Aug 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Thanks @nickofthyme !

@nickofthyme nickofthyme merged commit ee74c02 into elastic:master Aug 6, 2020
@nickofthyme nickofthyme deleted the upgrade-charts branch August 6, 2020 15:17
nyurik pushed a commit that referenced this pull request Aug 18, 2020
@nickofthyme @nyurik
Update elastic charts to 20.0.2 (#3792)
d13157e 
- update `@elastic/charts` to version `20.0.2`
- update `createTheme` function with theme changes
@thompsongl thompsongl mentioned this pull request Aug 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@cchaos cchaos cchaos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dependencies PRs that update a dependency file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nickofthyme @kibanamachine @cchaos @chandlerprall