-
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(partition): stroke configuration and linked label value font format #602
Conversation
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.
Checked locally and LGTM.
I've added a comment to improve the storybook
Codecov Report
@@ Coverage Diff @@
## master #602 +/- ##
==========================================
- Coverage 70.63% 70.35% -0.29%
==========================================
Files 220 220
Lines 6409 6432 +23
Branches 1228 1225 -3
==========================================
- Hits 4527 4525 -2
- Misses 1863 1888 +25
Partials 19 19
Continue to review full report at Codecov.
|
retest |
Jenkins retest |
jenkins retest this |
jenkins test this |
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.
LGTM, good to merge on green 💚
@@ -233,6 +234,7 @@ export const configMetadata = { | |||
// other | |||
backgroundColor: { dflt: '#ffffff', type: 'color' }, | |||
sectorLineWidth: { dflt: 1, min: 0, max: 4, type: 'number' }, | |||
sectorLineStroke: { dflt: 'white', type: 'string' }, |
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.
@monfera Sorry for looking at this late, but I'm confused by the name of this property and it's value. Does the stroke allow for more than just color?
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.
@cchaos at the moment it's just flat color; Canvas2d would allow pattern and gradient colors too, but it doesn't look super useful so it's not exposed. You're right, the name is not super specific, it's patterned after the CSS property https://css-tricks.com/almanac/properties/s/stroke/ and the current TypeScript is:
sectorLineStroke: StrokeStyle
...
type Color = string;
type StrokeStyle = Color; // now narrower than string | CanvasGradient | CanvasPattern
...
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.
K, thanks for the explanation!
# [18.2.0](v18.1.0...v18.2.0) (2020-03-26) ### Bug Fixes * **line_annotation:** keep the spec in state after chart rerender ([#605](#605)) ([43c13f1](43c13f1)), closes [#604](#604) ### Features * **partition:** stroke configuration and linked label value font format ([#602](#602)) ([7dce0a3](7dce0a3))
🎉 This PR is included in version 18.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [18.2.0](elastic/elastic-charts@v18.1.0...v18.2.0) (2020-03-26) ### Bug Fixes * **line_annotation:** keep the spec in state after chart rerender ([opensearch-project#605](elastic/elastic-charts#605)) ([22296c6](elastic/elastic-charts@22296c6)), closes [opensearch-project#604](elastic/elastic-charts#604) ### Features * **partition:** stroke configuration and linked label value font format ([opensearch-project#602](elastic/elastic-charts#602)) ([24d0e71](elastic/elastic-charts@24d0e71))
Summary
Feat: closes #603 by adding border color configurability, might be useful for non-white backgrounds.
With the former, white-only borders:
With new border color configuration, set to the almost-black background:
On a black background, with black borders, the sunburst may look more balanced with lighter colors.
Feat: closes #600 by adding value font formatting for linked labels
Closes #598 too (compare the new story before and after the 2nd commit)
Also closes #597 (5th commit)
Thanks @cchaos for the feature request and bug reports
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] Any consumer-facing exports were added tosrc/index.ts
(and stories only import from../src
except for test data & storybook)[ ] Unit tests were updated or added to match the most common scenarios