-
Notifications
You must be signed in to change notification settings - Fork 834
Charts: Add legend margin and styling customization props #45286
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
base: trunk
Are you sure you want to change the base?
Charts: Add legend margin and styling customization props #45286
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! |
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 adds customization options for legend appearance by introducing two new props: legendMargin
for controlling margin spacing and legendStyle
for custom CSS styling of legend containers.
- Add
legendMargin
andlegendStyle
props to BaseLegendProps interface with proper TypeScript definitions - Update BaseLegend component to apply margin and style props to legend wrapper elements
- Include comprehensive test coverage and Storybook controls for the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
types.ts | Adds TypeScript definitions for legendMargin and legendStyle props with JSDoc documentation |
base-legend.tsx | Implements the new props by applying margin and style to legend container |
legend.test.tsx | Adds comprehensive test cases for margin, styling, and their combination |
legend-config.tsx | Adds Storybook controls for legendMargin with descriptive documentation |
changelog | Documents the feature addition with minor significance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ts/js-packages/charts/changelog/charts-103-add-legend-margin-and-styling-customization-props
Outdated
Show resolved
Hide resolved
…rgin-and-styling-customization-props Co-authored-by: Copilot <[email protected]>
Code Coverage Summary2 files are newly checked for coverage.
|
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 10 out of 10 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/js-packages/charts/src/components/legend/private/base-legend.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/components/legend/private/base-legend.tsx
Outdated
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 13 out of 13 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/js-packages/charts/src/components/legend/utils/process-legend-margin.ts
Outdated
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 14 out of 14 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Like I commented here before, we probably don't want more props to control legend because there are already a few - Consumers could achieve similar styling by passing in a className.
I'll be more than happy to approve the PR if we allow passing in a legendItemClassName
so that consumers have full control of both the legend and the legend items.
As a follow up, this issue CHARTS-36 might be worth looking at to clean things up a bit.
More customization could be achieved with this one CHARTS-119.
Proposed changes:
legendMargin
prop to BaseLegendProps interface to allow custom margin spacing around legendslegendStyle
prop to BaseLegendProps interface to enable custom CSS styling of legend containerslegendMargin
andlegendStyle
props in legend configurationOther information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
legendMargin
control to test different margin values:8
,16
,24
for uniform margins{top: 10, right: 15, bottom: 10, left: 15}
for custom marginslegendStyle
control to test custom styling:{backgroundColor: '#f0f0f0'}
{border: '1px solid #ccc'}
{padding: '8px'}