fix: #3073 #3862
fix: #3073 #3862
Conversation
HHongSeungWoo
force-pushed
the
fix/3859
branch
from
f2a671a
to
0171b05
Compare
|
@HHongSeungWoo do you know why this worked on some browsers and not others? |
|
@ckifer If you are referring to Firefox DevTools, Firefox's mobile mode does not simulate mobile devices like Chrome does. It shows mobile pages but triggers mouse events. |
|
Oh wow that's good to know 🙃. Cool, will review this more later |
| @@ -1116,8 +1114,7 @@ export const generateCategoricalChart = ({ | |||
| constructor(props: CategoricalChartProps) { | |||
| super(props); | |||
|
|
|||
| this.uniqueChartId = _.isNil(props.id) ? uniqueId('recharts') : props.id; | |||
| this.clipPathId = `${this.uniqueChartId}-clip`; | |||
| this.clipPathId = `${props.id ?? uniqueId('recharts')}-clip`; | |||
There was a problem hiding this comment.
Nice. Love the small cleanups.
| if (!_.isNil(syncId)) { | ||
| eventCenter.emit(SYNC_EVENT, syncId, this.uniqueChartId, data); | ||
| } |
There was a problem hiding this comment.
Same here, why can we remove this? Can the syncId no longer be null?
There was a problem hiding this comment.
How are we making sure it is not null? Could a user not still set it to null?
There was a problem hiding this comment.
It was just a missed. Thank you for finding
| @@ -751,7 +750,7 @@ describe('<LineChart /> - Rendering two line charts with syncId', () => { | |||
| expect(container.querySelectorAll('.recharts-tooltip-cursor')).toHaveLength(2); | |||
|
|
|||
| // make sure tooltips display the correct values, synced by data value | |||
| expect(screen.queryByText('400')).toBeTruthy(); | |||
| expect(screen.queryByText('300')).toBeTruthy(); | |||
There was a problem hiding this comment.
This is surprising. Did the test previously capture behaviour that was a bug itself?
Could you please add a storybook test (i.e. in a play function for a story with syncId) to verify that the syncId still works correctly?
There was a problem hiding this comment.
Great, I'll give it a try.
|
|
||
| if (!_.isNil(syncId)) { | ||
| eventCenter.emit(SYNC_EVENT, syncId, this.uniqueChartId, data); | ||
| if (this.props.syncId !== undefined) { |
There was a problem hiding this comment.
| if (this.props.syncId !== undefined) { | |
| if (this.props.syncId != undefined) { |
I assume we would also want to catch null, right?
There was a problem hiding this comment.
No, there shouldn't be null. Typing errors should be caught by TypeScript.
<!--- Provide a general summary of your changes in the Title above --> I have fixed the bug. before https://github.com/recharts/recharts/assets/34399997/6cae6da9-d761-4864-a3a5-442b0e6d3fa5 after https://github.com/recharts/recharts/assets/34399997/6156aa9c-eeaf-4b95-bbe0-bd77c8c5dbde <!--- Describe your changes in detail --> ## Related Issue recharts#3073 <!--- 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: --> ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> ## 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. --> ## Screenshots (if appropriate): ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] 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! --> - [x] 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 - [x] All new and existing tests passed.
I have fixed the bug.
before
bug.mp4
after
fix.mp4
Related Issue
#3073
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: