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

Update elastic charts to 20.0.2 #3792

Merged
merged 13 commits into from
Aug 6, 2020
Merged

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Jul 23, 2020

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

- update `createTheme` function with theme changes
- add `@elastic/charts` under `optionalDependencies`
@nickofthyme nickofthyme added the dependencies Pull requests that update a dependency file label Jul 23, 2020
package.json Outdated Show resolved Hide resolved
src/components/common.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
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?

CHANGELOG.md Outdated Show resolved Hide resolved
@nickofthyme
Copy link
Contributor Author

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

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

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

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Jul 23, 2020

@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
Contributor Author

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

image

@nickofthyme
Copy link
Contributor Author

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

@cchaos
Copy link
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

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Aug 4, 2020

retest

@cchaos cchaos requested a review from chandlerprall August 4, 2020 20:47
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.

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@nickofthyme
Copy link
Contributor Author

@chandlerprall are you ok with these changes?

Copy link
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
- update `@elastic/charts` to version `20.0.2`
- update `createTheme` function with theme changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants