refactor(wordcloud): cleanup config, types and theming (#1358)#1407
refactor(wordcloud): cleanup config, types and theming (#1358)#1407nickofthyme merged 7 commits intoelastic:masterfrom
Conversation
BREAKING CHANGES: The `WorkcloudSpec.config` prop is removed as it was not used other than assigning `margins` even with erroneous properties. All wordcloud properties are now driven from the `WorkcloudSpec` directly. Since the wordcloud is unique in that it's styles are driven by the data I think keeping them on the spec is more favorable than moving them to the theme as they would be overridden more frequently. This does not provide a themed instance of the chart type but this could possibly come from `.brightening` the provided colors of the text elements. Changes include - `WorkcloudSpec.margin` deleted in favor of `Theme.chartMargins`. - `WorkcloudConfigs` is removed in favor of singular `WordcloudViewModel` type which is extended to form `WordcloudSpec`.
nickofthyme
added
:styling
storybook/style.scss
Outdated
| @@ -1,4 +1,5 @@ | |||
| @import '~@elastic/eui/dist/eui_theme_light'; | |||
| @import '~@elastic/charts/src/theme_light'; | |||
There was a problem hiding this comment.
This is recommended by EUI team to fix base styles. Causing changes to integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-should-render-current-tooltip-in-dark-theme-1-snap.png.
There was a problem hiding this comment.
In the screenshot, it looks like it's using the light theme tooltip color instead of the dark one.
There is also another difference: this is not the same result as it appear on storybook (in storybook the tooltip bg color is black, not dark gray)
There was a problem hiding this comment.
@nickofthyme before merging please check my comment above
monfera
left a comment
monfera
left a comment
There was a problem hiding this comment.
LGTM, read through the code and still like the code noise removals ✂️
| const getTemplate = (name: string): WordcloudKnobs => { | ||
| switch (name) { | ||
| case 'single': |
There was a problem hiding this comment.
What is the benefit of changing this to a function?
There was a problem hiding this comment.
easier typing but yeah doesn't really make much difference.
storybook/style.scss
Outdated
| @@ -1,4 +1,5 @@ | |||
| @import '~@elastic/eui/dist/eui_theme_light'; | |||
| @import '~@elastic/charts/src/theme_light'; | |||
There was a problem hiding this comment.
In the screenshot, it looks like it's using the light theme tooltip color instead of the dark one.
There is also another difference: this is not the same result as it appear on storybook (in storybook the tooltip bg color is black, not dark gray)
|
@elasticmachine merge upstream |
4 participants
Summary
Refactor
wordcloudspec to cleanup duplicate or unused config and types.BREAKING CHANGES
The
WorkcloudSpec.configprop is removed as it was not used other than assigningmarginseven with erroneous properties. All wordcloud properties are now driven from theWorkcloudSpecdirectly. Since the wordcloud is unique in that it's styles are driven by the data I think keeping them on the spec is more favorable than moving them to the theme as they would be overridden more frequently. This does not provide a themed instance of the chart type but this could possibly come from.brighteningthe provided colors of the text elements.Changes include
WorkcloudSpec.margindeleted in favor ofTheme.chartMargins.WorkcloudConfigsis removed in favor of singularWordcloudViewModeltype which is extended to formWordcloudSpec.Details
Applies #1358 to
masterOther Changes:
Wordtypes are refined to reflect the mutating nature ofd3TagCloudfunction.d3-cloudChecklist
:xy,:partition):interactions,:axis):themelabel has been added and the@elastic/eui-designteam has been pinged when there areThemeAPI changescloses #123,fixes #123)packages/charts/src/index.tsdark,light,eui-dark&eui-light