-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat(theme): allow recursive partial theme #239
Conversation
This comment has been minimized.
This comment has been minimized.
yarn.lock
Outdated
@@ -4951,7 +4951,7 @@ deep-is@~0.1.3: | |||
resolved "https://registry.yarnpkg.com/deep-is/-/deep-is-0.1.3.tgz#b369d6fb5dbc13eecf524f91b070feedc357cf34" | |||
integrity sha1-s2nW+128E+7PUk+RsHD+7cNXzzQ= | |||
|
|||
[email protected]: | |||
[email protected], deepmerge@^3.2.0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markov00 it looks like we are already using this library through some dependency. Not sure if that matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it's semantic-release via octokit... important thing is that we don't use it directly
The main tsconfig.json is what vscode uses to parse ts files. Adding a global type would not available in excluded test files. Extended base tsconfig to current match current build config.
Allow recursive partial themes to be passed to the Setting component. Added RecursivePartial global type. Issue elastic#201
1490d31
to
257f649
Compare
Codecov Report
@@ Coverage Diff @@
## master #239 +/- ##
==========================================
- Coverage 97.75% 97.74% -0.01%
==========================================
Files 36 36
Lines 2626 2621 -5
Branches 587 582 -5
==========================================
- Hits 2567 2562 -5
Misses 52 52
Partials 7 7
Continue to review full report at Codecov.
|
yarn.lock
Outdated
@@ -4951,7 +4951,7 @@ deep-is@~0.1.3: | |||
resolved "https://registry.yarnpkg.com/deep-is/-/deep-is-0.1.3.tgz#b369d6fb5dbc13eecf524f91b070feedc357cf34" | |||
integrity sha1-s2nW+128E+7PUk+RsHD+7cNXzzQ= | |||
|
|||
[email protected]: | |||
[email protected], deepmerge@^3.2.0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it's semantic-release via octokit... important thing is that we don't use it directly
Moved global type to be utils. Updated storybook config with custom tsconfig file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally and code LGTM!
definitely doesn't need to be handled in this PR but would be nice to also extend this to allow for partial style definitions for the per series custom styles.
🎉 This PR is included in version 6.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [6.3.0](elastic/elastic-charts@v6.2.0...v6.3.0) (2019-06-20) ### Features * **theme:** allow recursive partial theme ([opensearch-project#239](elastic/elastic-charts#239)) ([64e9451](elastic/elastic-charts@64e9451)), closes [opensearch-project#201](elastic/elastic-charts#201)
Summary
Issue #201
Allow partial theme object for chart
Settings
You can now specify a
PartialTheme
to pass to theSettings
component where every field in the fullTheme
type is recursively optional.Before
If you wanted to change one property in a nested type lets say
left
margin we want to set to0
. You would need to include the default/base values ofright
,top
andbottom
margins for those to carry over, otherwise, they will be removed!After
You ONLY need to include the values you want, then pass that to the
Settings
component.There is also no need to merge this
PartialTheme
theme using themergeWithDefaultTheme
function, although you can if you really want to 😏. This will happen automatically within theSettings
. By default, it will deep-merge yourPartialTheme
with theLIGHT_THEME
as a base. However, if you would like to set the base to be something else you can pass that as a prop.Checklist
src/index.ts
(and stories only import from../src
except for test data & storybook)