Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Funnel support custom shape prop #3886

Merged
merged 6 commits into from Oct 26, 2023
Merged

feat: Funnel support custom shape prop #3886

merged 6 commits into from Oct 26, 2023

Conversation

andrewangelle
Copy link
Contributor

Description

Add support for shape prop in Funnel for user to provide a customized shape.

Related Issue

#3832

Motivation and Context

Unified api for shape/activeShape support between Funnel/Line/Bar/RadialBar/Scatter

How Has This Been Tested?

unit tests and storybook

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a storybook story or extended an existing story to show my changes
  • All new and existing tests passed.

@andrewangelle andrewangelle mentioned this pull request Oct 20, 2023
Merged
ckifer
ckifer requested changes Oct 20, 2023
View reviewed changes
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to see if we can remove that cast

src/util/ActiveShapeUtils.tsx Show resolved Hide resolved
src/util/ActiveShapeUtils.tsx Outdated

if (isValidElement(option)) {
shape = cloneElement(option, props);
nextProps = propTransformer(option.props as OptionType, props);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way we don't have to cast here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like there is. 😃

ckifer
ckifer reviewed Oct 20, 2023
View reviewed changes
src/util/ActiveShapeUtils.tsx Outdated
Comment on lines 88 to 90
if (isValidElement<OptionType>(option)) {
nextProps = propTransformer(option.props, props);
shape = cloneElement<OptionType>(option, nextProps);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though we remove as this is still casting.

Its probably fine since we're doing all of the checks we do here but still

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made another update check again, please.

nikolasrieble
nikolasrieble reviewed Oct 21, 2023
View reviewed changes
storybook/stories/API/chart/FunnelChart.stories.tsx Outdated
Comment on lines 20 to 29
shape: {
description: 'The customized shape to be rendered.',
table: {
type: {
summary: 'Function | boolean | ReactElement | object',
},
defaultValue: undefined,
category: 'General',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is documented in multiple places. Should we extract a shared description, and import it?

That would enforce standardised user facing communication for this property.

nikolasrieble
nikolasrieble reviewed Oct 21, 2023
View reviewed changes
test/chart/FunnelChart.spec.tsx Outdated
@@ -229,4 +229,55 @@ describe('<FunnelChart />', () => {
const activeShape = container.querySelectorAll('.recharts-active-shape');
expect(activeShape).toHaveLength(0);
});

test('renders customized shape when shape is set to be a function', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a curious idea:

How about instead of duplicating these tests per component, why not create a single test file, for shape, and test all supported components at once with a parametrised test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would further enforce standardised behaviour and reduce copy pasted code /test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this for the Funnel cases, can you take a look and confirm I'm on the right path. If so, I'll convert the others.

ckifer
ckifer approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

nikolasrieble
nikolasrieble approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@nikolasrieble nikolasrieble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for cleaning up the tests! Appreciate it!

@nikolasrieble nikolasrieble merged commit a7eb2ab into recharts:master Oct 26, 2023
5 checks passed
GMer78 pushed a commit to GMer78/recharts-1 that referenced this pull request Nov 24, 2023
@andrewangelle
feat: Funnel support custom shape prop (recharts#3886)
bed77b2 
## Description

Add support for shape prop in Funnel for user to provide a customized
shape.

## Related Issue

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please link to the issue here: -->
recharts#3832 
## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
Unified api for shape/activeShape support between
Funnel/Line/Bar/RadialBar/Scatter

## How Has This Been Tested?
unit tests and storybook
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Screenshots (if appropriate):

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [x] I have added a storybook story or extended an existing story to
show my changes
- [x] All new and existing tests passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckifer ckifer ckifer approved these changes

@nikolasrieble nikolasrieble nikolasrieble approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andrewangelle @ckifer @nikolasrieble