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

[WIP] Part-to-whole comparison chart guidelines #3387

Merged
merged 39 commits into from
May 8, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 24, 2020

Upgrades Charts to v. 19.2.0

Pie/Donut/Sunburst/Treemap guidelines

Mostly talks about Pie/Donut charts specifically, but the guidelines apply to most "part-to-whole" comparison charts.

Screen Shot 2020-05-04 at 17 18 54 PM


Palette changes

  1. euiPaletteColorBlind() now accepts two other parameters.
  • sortBy: 'default' | 'natural' for sorting the colors based on the color wheel
  • sortShift: number for shifting the starting point of the color wheel (defaults to the green position -100
  1. The EuiChartThemeType now has a partition key specifically for Partition charts that have specific configs.

Breaking Changes

A. The euiPaletteColorBlind() function was becoming unwieldly with the amount of parameters it can accept. It just wasn't scalable as a simple parameters. Now, the function has one parameter of the object type.

// Before
euiPaletteColorBlind(2)
// After
euiPaletteColorBlind({ rotations: 2 })

B. The sort order for the EUI_CHARTS_THEME_LIGHT/DARK color blind palette now defaults to the "natural" sort order. This means all EUI themed charts, will have a slightly different sorting of colors.

Screen Shot 2020-05-04 at 17 00 38 PM

Note to consumers: While this is a breaking change, unless you used the default sort order to match your categories against, there shouldn't be a significant difference.


Other changes

i. Updates the homepage card listing to add one for Charts, Text, and Page layout. Also, I exported the images slightly different than before so they're a bit better looking in dark mode.

Screen Shot 2020-05-04 at 16 51 37 PM

ii. Adds RecursivePartial<> TS utility for allowing recursive object keys to be optional

iii. Changed EuiImage's caption prop to also allow for non-strings, like html elements. I ran into this trying to pass <small>Caption</small> as the prop to make the text smaller.


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

@cchaos
Copy link
Contributor Author

cchaos commented Apr 24, 2020

retest

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor

Can you add this page to the a11y tests? (I realized we should have been doing this on every PR that adds a new page until the testing runs all page by default...)

Looks like there'll be just one duplicate ID issue to fix.

@kibanamachine
Copy link

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

@cchaos cchaos marked this pull request as ready for review May 4, 2020 21:08
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@snide
Copy link
Contributor

snide commented May 5, 2020

Couple quick ones for dark mode. Will do a larger code / copy review tomorrow.

Dark mode

image

image

image

@cchaos cchaos requested review from miukimiu, thompsongl and snide and removed request for miukimiu May 5, 2020 12:59
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

The comments I got on pie charts in Lens indicated that the correct default is to show the percent value instead of the numeric value inside the slices, and since it's already supported by the chart library I think you could show both options.

src-docs/src/views/elastic_charts/treemap.js Show resolved Hide resolved
@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Amazing work! I really like all the explanations and all the interactions. 😍

I just found two small typos. All the rest looks good to me! 🎉

src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_alts.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This is fantastic. Left some small grammatical suggestions. Verified previous comments about dark mode were fixed. Looks good to merge once you're happy with it.

src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

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.

Thank you Caroline for this, I've left a few minor comments but everything else is great.

package.json Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src-docs/src/views/elastic_charts/pie_example.js Outdated Show resolved Hide resolved
src/components/common.ts Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented May 8, 2020

retest

@kibanamachine
Copy link

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

@cchaos cchaos merged commit 826bdb5 into elastic:master May 8, 2020
@cchaos cchaos deleted the docs/charts/pie branch September 11, 2020 15:39
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.

None yet

7 participants