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
fix: #3073 #3862
fix: #3073 #3862
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1091,8 +1091,6 @@ export const generateCategoricalChart = ({ | |
return class CategoricalChartWrapper extends Component<CategoricalChartProps, CategoricalChartState> { | ||
static displayName = chartName; | ||
|
||
uniqueChartId: string; | ||
|
||
clipPathId: string; | ||
|
||
cancelDefer: CancelFunction | null; | ||
|
@@ -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`; | ||
|
||
if (props.throttleDelay) { | ||
this.triggeredAfterMouseMove = _.throttle(this.triggeredAfterMouseMove, props.throttleDelay); | ||
|
@@ -1423,21 +1420,12 @@ export const generateCategoricalChart = ({ | |
}; | ||
} | ||
|
||
/* eslint-disable no-underscore-dangle */ | ||
addListener() { | ||
eventCenter.on(SYNC_EVENT, this.handleReceiveSyncEvent); | ||
|
||
if (eventCenter.setMaxListeners && eventCenter._maxListeners) { | ||
eventCenter.setMaxListeners(eventCenter._maxListeners + 1); | ||
} | ||
} | ||
|
||
removeListener() { | ||
eventCenter.removeListener(SYNC_EVENT, this.handleReceiveSyncEvent); | ||
|
||
if (eventCenter.setMaxListeners && eventCenter._maxListeners) { | ||
eventCenter.setMaxListeners(eventCenter._maxListeners - 1); | ||
} | ||
ckifer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
clearDefer = () => { | ||
|
@@ -1466,10 +1454,8 @@ export const generateCategoricalChart = ({ | |
} | ||
}; | ||
|
||
handleReceiveSyncEvent = (cId: number | string, chartId: string, data: CategoricalChartState) => { | ||
const { syncId } = this.props; | ||
|
||
if (syncId === cId && chartId !== this.uniqueChartId) { | ||
handleReceiveSyncEvent = (cId: number | string, data: CategoricalChartState) => { | ||
if (this.props.syncId === cId) { | ||
this.clearDefer(); | ||
this.cancelDefer = deferer(this.applySyncEvent.bind(this, data)); | ||
} | ||
|
@@ -1659,11 +1645,7 @@ export const generateCategoricalChart = ({ | |
}; | ||
|
||
triggerSyncEvent(data: CategoricalChartState) { | ||
const { syncId } = this.props; | ||
|
||
if (!_.isNil(syncId)) { | ||
eventCenter.emit(SYNC_EVENT, syncId, this.uniqueChartId, data); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, why can we remove this? Can the syncId no longer be null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was just a missed. Thank you for finding |
||
eventCenter.emit(SYNC_EVENT, this.props.syncId, data); | ||
} | ||
|
||
applySyncEvent(data: CategoricalChartState) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import React, { FC } from 'react'; | ||
import { render, fireEvent, screen } from '@testing-library/react'; | ||
import type { JSX } from '@babel/types'; | ||
|
||
import { LineChart, Line, XAxis, YAxis, Tooltip, Brush, CartesianAxis, Legend } from '../../src'; | ||
|
||
|
@@ -393,9 +392,9 @@ describe('<LineChart />', () => { | |
describe('<LineChart /> - Pure Rendering', () => { | ||
const pureElements = [Line]; | ||
|
||
const spies: Array<jest.SpyInstance<JSX.Element | null, []>> = []; | ||
const spies: Array<jest.SpyInstance<React.ReactElement | null, []>> = []; | ||
// CartesianAxis is what is actually render for XAxis and YAxis | ||
let axisSpy: jest.SpyInstance<JSX.Element | null, []>; | ||
let axisSpy: jest.SpyInstance<React.ReactElement | null, []>; | ||
|
||
// spy on each pure element before each test, and restore the spy afterwards | ||
beforeAll(() => { | ||
|
@@ -475,9 +474,9 @@ describe('<LineChart /> - Pure Rendering', () => { | |
describe('<LineChart /> - Pure Rendering with legend', () => { | ||
const pureElements = [Line]; | ||
|
||
const spies: Array<jest.SpyInstance<JSX.Element | null, []>> = []; | ||
const spies: Array<jest.SpyInstance<React.ReactElement | null, []>> = []; | ||
// CartesianAxis is what is actually render for XAxis and YAxis | ||
let axisSpy: jest.SpyInstance<JSX.Element | null, []>; | ||
let axisSpy: jest.SpyInstance<React.ReactElement | null, []>; | ||
|
||
// spy on each pure element before each test, and restore the spy afterwards | ||
beforeAll(() => { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is surprising. Did the test previously capture behaviour that was a bug itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, I'll give it a try. |
||
expect(screen.queryByText('550')).toBeTruthy(); | ||
|
||
// Check the activeDots are highlighted | ||
|
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.
Nice. Love the small cleanups.