Skip to content

Conversation

@nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Aug 20, 2019

Summary

Allow baseTheme prop to set base theme.

#292

BREAKING CHANGE: remove baseThemeType prop on Settings Component and BaseThemeTypes type.

Checklist

  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Each commit follows the convention

Allow baseTheme prop to override baseThemeType prop

elastic#292
@nickofthyme nickofthyme requested a review from markov00 August 20, 2019 19:20
@codecov-io
Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #333 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
- Coverage   98.51%   98.51%   -0.01%     
==========================================
  Files          38       38              
  Lines        2698     2691       -7     
  Branches      635      620      -15     
==========================================
- Hits         2658     2651       -7     
  Misses         37       37              
  Partials        3        3
Impacted Files Coverage Δ
src/chart_types/xy_chart/store/chart_state.ts 97.1% <ø> (-0.02%) ⬇️
src/utils/themes/theme.ts 100% <ø> (ø) ⬆️
src/specs/settings.tsx 96.34% <100%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abaa472...7d5540c. Read the comment docs.

@nickofthyme nickofthyme added :chart Chart element related issue :styling Styling related issue enhancement New feature or request labels Aug 20, 2019
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.

Code LGTM. I think, in a major release, we can remove the baseThemeType that, as it is, is a bit redundant now

@nickofthyme
Copy link
Collaborator Author

Yup I just think a breaking change would not be needed.

@nickofthyme
Copy link
Collaborator Author

@markov00 what do you think of setting the default theme in Settings not the chart_state?

/**
* Chart theme to be set from Settings.tsx
*/
chartTheme!: Theme;

chartStore.chartTheme = getTheme(baseTheme, theme);

see 7d5540c

@nickofthyme nickofthyme merged commit a9ff5e1 into elastic:master Aug 21, 2019
markov00 pushed a commit that referenced this pull request Aug 21, 2019
# [10.0.0](v9.2.1...v10.0.0) (2019-08-21)

### Bug Fixes

* **tooltip:** ie11 flex sizing ([#334](#334)) ([abaa472](abaa472)), closes [#332](#332)
* decuple brush cursor from chart rendering ([#331](#331)) ([789f85a](789f85a)), closes [elastic/kibana#36517](elastic/kibana#36517)
* remove clippings from chart geometries ([#320](#320)) ([ed6d0e5](ed6d0e5)), closes [#20](#20)

### Features

* auto legend resize ([#316](#316)) ([659d27e](659d27e)), closes [#268](#268)
* customize number of axis ticks ([#319](#319)) ([2b838d7](2b838d7))
* **theme:** base theme prop ([#333](#333)) ([a9ff5e1](a9ff5e1)), closes [#292](#292)

### BREAKING CHANGES

* **theme:** remove `baseThemeType` prop on `Settings` component and `BaseThemeTypes` type.
* `theme.legend.spacingBuffer` added to `Theme` type. Controls the width buffer between the legend label and value.
@markov00
Copy link
Member

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Aug 21, 2019
@nickofthyme nickofthyme deleted the feat/base-theme branch August 26, 2019 19:59
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [10.0.0](elastic/elastic-charts@v9.2.1...v10.0.0) (2019-08-21)

### Bug Fixes

* **tooltip:** ie11 flex sizing ([opensearch-project#334](elastic/elastic-charts#334)) ([2683333](elastic/elastic-charts@2683333)), closes [opensearch-project#332](elastic/elastic-charts#332)
* decuple brush cursor from chart rendering ([opensearch-project#331](elastic/elastic-charts#331)) ([b5b8dde](elastic/elastic-charts@b5b8dde)), closes [elastic/kibana#36517](elastic/kibana#36517)
* remove clippings from chart geometries ([opensearch-project#320](elastic/elastic-charts#320)) ([497efa4](elastic/elastic-charts@497efa4)), closes [opensearch-project#20](elastic/elastic-charts#20)

### Features

* auto legend resize ([opensearch-project#316](elastic/elastic-charts#316)) ([be4a53d](elastic/elastic-charts@be4a53d)), closes [opensearch-project#268](elastic/elastic-charts#268)
* customize number of axis ticks ([opensearch-project#319](elastic/elastic-charts#319)) ([a7a4ecd](elastic/elastic-charts@a7a4ecd))
* **theme:** base theme prop ([opensearch-project#333](elastic/elastic-charts#333)) ([0b38770](elastic/elastic-charts@0b38770)), closes [opensearch-project#292](elastic/elastic-charts#292)

### BREAKING CHANGES

* **theme:** remove `baseThemeType` prop on `Settings` component and `BaseThemeTypes` type.
* `theme.legend.spacingBuffer` added to `Theme` type. Controls the width buffer between the legend label and value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:chart Chart element related issue enhancement New feature or request released Issue released publicly :styling Styling related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants