-
Notifications
You must be signed in to change notification settings - Fork 834
Charts: Improve Storybook organization, add configuration stories and fix control issues #45405
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
Charts: Improve Storybook organization, add configuration stories and fix control issues #45405
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
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.
Pull Request Overview
This PR consolidates redundant Storybook stories into interactive configuration stories with controls, reducing story count while improving discoverability. The changes replace multiple static stories with single stories featuring interactive controls for tooltips, annotations, glyphs, legends, and chart orientations.
Key Changes:
- Replaces 5 Line Chart tooltip stories with 1 interactive TooltipConfiguration story
- Replaces 4 Line Chart annotation stories with 1 AnnotationConfiguration story
- Adds comprehensive GlyphConfiguration story with glyph positioning and type controls
- Consolidates legend stories for both Line and Pie charts into unified LegendConfiguration stories
- Enhances Bar Chart Default story with orientation and grid visibility controls
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pie-chart/stories/index.stories.tsx |
Replaces separate legend stories with unified LegendConfiguration featuring 8 interactive controls |
line-chart/stories/tooltip.stories.tsx |
Consolidates 5 tooltip stories into single TooltipConfiguration with crosshair controls |
line-chart/stories/index.stories.tsx |
Replaces legend stories with comprehensive LegendConfiguration story |
line-chart/stories/glyph.stories.tsx |
Adds new GlyphConfiguration story with positioning, type, and size controls |
line-chart/stories/annotation.stories.tsx |
Consolidates 4 annotation stories into AnnotationConfiguration with subject type controls |
bar-chart/stories/index.stories.tsx |
Enhances Default story with orientation and grid visibility controls |
changelog/charts-37-use-the-controls-to-put-similar-stories-together |
Documents the consolidation changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/js-packages/charts/src/components/line-chart/stories/glyph.stories.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/components/line-chart/stories/glyph.stories.tsx
Outdated
Show resolved
Hide resolved
4c45afd
to
e58bb7d
Compare
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.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
projects/js-packages/charts/src/components/line-chart/stories/glyph.stories.tsx:1
- The GlyphStar component expects size * size for the size property, but this is passing the raw size value. This inconsistency with the visx API could cause glyphs to render incorrectly.
import { GlobalChartsProvider } from '../../../providers';
projects/js-packages/charts/src/components/line-chart/stories/glyph.stories.tsx:1
- The GlyphStar component expects size * size for the size property. The raw size value being passed could result in smaller than expected glyphs.
import { GlobalChartsProvider } from '../../../providers';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/js-packages/charts/src/components/line-chart/stories/config.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/components/line-chart/stories/config.tsx
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
bfde220
to
1df5a36
Compare
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.
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.
We need to double check whether some controls are applicable for all the stories - if a control doesn't work, then we should either remove it or make it work. |
I agree! Sorry I got this ready then made some changes on Friday evening, I should've marked it back as a draft while I fixed it up 🙈 I'm working on it right now though and will let you know once it's actually ready for review this time. |
1df5a36
to
7d69934
Compare
…es, hide inapplicable controls
… and Vertical Legend
This got out of control! Closing up in favour of the much cleaner & simplified #45503 |
fixes: #CHARTS-37
Proposed changes:
This PR improves Storybook organization by adding Configuration stories with interactive controls and fixing control visibility issues across all chart components.
Configuration Stories with Interactive Controls
Control Visibility Improvements
Bug Fixes
Documentation Updates
Control Organization
Impact
Other information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
Bar Chart - Configuration Story
Bar Chart - Documentation Examples
Bar List Chart - Configuration
Leaderboard Chart - Configuration
Line Chart - Configuration
Pie Chart - Controls
Overall Verification