-
Notifications
You must be signed in to change notification settings - Fork 825
Charts- 81: Leaderboard chart supports custom label for greater flexibility. #44751
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- 81: Leaderboard chart supports custom label for greater flexibility. #44751
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 SummaryCoverage changed in 1 file.
|
e88773d
to
bd5974a
Compare
Happy not to add additional props and I think we don't need that at the stage. We could use the border radius in the design for bars when the pictures are in place.
I prefer to start simple 🙂
Let's fully achieve that as a story to showcase it. |
bd5974a
to
fe22c94
Compare
fb3ee5a
to
8f1112b
Compare
8f1112b
to
a6f3d81
Compare
556cb22
to
2aaf7f4
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.
Looks great! Added non-blocking suggestions
projects/js-packages/charts/src/components/leaderboard-chart/stories/index.stories.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/components/leaderboard-chart/stories/index.stories.tsx
Show resolved
Hide resolved
{ typeof entry.label === 'string' ? <Text>{ entry.label }</Text> : entry.label } | ||
|
||
<div className={ styles.progressContainer }> | ||
<ProgressBar |
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.
It seems ProgressBar wouldn't satisfy the needs here, so we should probably just use our custom implementation in ProgressBarWithOverlayLabel.
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.
border-radius: var(--bar-border, 9999px); | ||
z-index: -1; /* places it behind the label */ | ||
|
||
transition: width 0.3s ease-in-out; |
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.
Let's not use animation yet.
Fixes #CHARTS-81
Proposed changes:
JSX.Element
as label to enable more customization. It’s common for labels to include pre/post icons, custom styles etc. This approach makes the label for customizable and flexible.Added another boolean prop
withOverlayLabel
which overlays the label on top of each corresponding bar.withOverlayLabel
.withOverlayLabel
andwithComparison
.withOverlayLabel
.Note that the
--bar-border
custom CSS property has been introduced to control the border of each bar. While a custom CSS property may be less discoverable than a prop, this is a niche use case, and we can always introduce it as a prop if needed.Also, the bar height is determined by the label’s height, while its width is based on
currentShare
and the overall width of the leaderboard component.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: