Skip to content

Commit

Permalink
fix(clipDot): proper defaults for r and strokeWidth when they aren't …
Browse files Browse the repository at this point in the history
…provided by the user (#3624)

<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->
Basically
https://codesandbox.io/s/simple-line-chart-forked-4gczxs?file=/src/App.tsx

When `allowDataOverflow` is true on an axis and `clipDot` is false on a
`dot` (in a Line or Area) without `r` or `strokeWidth` the calculations
[here](https://github.com/recharts/recharts/blob/bf8d626c76c0534fd2a762c0859d6be9e1459e37/src/cartesian/Line.tsx#L468)
cause `NaN` to be added on the `clipPath` and cause console errors. This
is because the defaults just above that do not work if the user provides
their own props for dot i.e. `dot={{ clipDot: false }}`.

Fix this by defaulting the params to the same values they default to if
the user provides no params.

## 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: -->
#3602
PR where this was merged, no issue yet. Follow up from release

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
Fix bug released in 2.7

## How Has This Been Tested?

<!--- 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. -->
`clipPath` for dots has valid values and does not add attributes with
NaN.

* no more console errors
* add unit test to confirm this for line

## Screenshots (if appropriate):
<img width="505" alt="image"
src="https://github.com/recharts/recharts/assets/25180830/b89acae0-db57-41d4-ab92-41fdc4b2a616">


## Types of changes

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

- [Y] 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:

<!--- 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! -->

- [Y] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [Y] I have added tests to cover my changes.
- [Y] All new and existing tests passed.

Co-authored-by: Coltin Kifer <ckifer@amazon.com>
  • Loading branch information
ckifer and Coltin Kifer committed Jun 15, 2023
1 parent 96fe57f commit 75e4c67
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/cartesian/Area.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ export class Area extends PureComponent<Props, State> {
const needClipY = yAxis && yAxis.allowDataOverflow;
const needClip = needClipX || needClipY;
const clipPathId = _.isNil(id) ? this.id : id;
const { r, strokeWidth } = filterProps(dot) || { r: 3, strokeWidth: 2 };
const { r = 3, strokeWidth = 2 } = filterProps(dot) ?? { r: 3, strokeWidth: 2 };
const { clipDot = true } = isDotProps(dot) ? dot : {};
const dotSize = r * 2 + strokeWidth;

Expand Down
2 changes: 1 addition & 1 deletion src/cartesian/Line.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ export class Line extends PureComponent<Props, State> {
const needClipY = yAxis && yAxis.allowDataOverflow;
const needClip = needClipX || needClipY;
const clipPathId = _.isNil(id) ? this.id : id;
const { r, strokeWidth } = filterProps(dot) || { r: 3, strokeWidth: 2 };
const { r = 3, strokeWidth = 2 } = filterProps(dot) ?? { r: 3, strokeWidth: 2 };
const { clipDot = true } = dot as DotProps;
const dotSize = r * 2 + strokeWidth;

Expand Down
1 change: 1 addition & 0 deletions storybook/stories/Examples/LineChart.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ export const ClipDot: StoryObj = {
{...args}
dot={{ clipDot: args.clipDot, r: 4, strokeWidth: 2, fill: '#ffffff', fillOpacity: 1 }}
/>
<Line isAnimationActive={false} dataKey="pv" {...args} dot={{ clipDot: args.clipDot }} />
<Tooltip />
<XAxis dataKey="name" allowDataOverflow />
<YAxis />
Expand Down
26 changes: 26 additions & 0 deletions test/chart/LineChart.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -762,4 +762,30 @@ describe('<LineChart /> - Rendering two line charts with syncId', () => {
jest.runAllTimers();
expect(container.querySelectorAll('.recharts-active-dot')).toHaveLength(0);
});

test('Render a line with clipDot option on the dot and expect attributes not to be NaN', () => {
const { container } = render(
<LineChart width={width} height={height} data={data2}>
<Line type="monotone" dataKey="uv" stroke="#ff7300" dot={{ clipDot: false }} />
<Tooltip />
<XAxis dataKey="name" allowDataOverflow />
</LineChart>,
);

expect(container.querySelectorAll('.recharts-line-curve')).toHaveLength(1);
const clipPaths = container.getElementsByTagName('clipPath');
for (let i = 0; i < clipPaths.length; i++) {
const clipPath = clipPaths.item(i);
const rects = clipPath && clipPath.getElementsByTagName('rect');
for (let j = 0; j < clipPaths.length; j++) {
const rect = rects?.item(j);
if (rect) {
expect(Number(rect.getAttribute('height'))).not.toBeNaN();
expect(Number(rect.getAttribute('width'))).not.toBeNaN();
expect(Number(rect.getAttribute('x'))).not.toBeNaN();
expect(Number(rect.getAttribute('y'))).not.toBeNaN();
}
}
}
});
});

0 comments on commit 75e4c67

Please sign in to comment.