Skip to content

Commit

Permalink
fix: activeBar shouldn't be true by default (#4139)
Browse files Browse the repository at this point in the history
<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->
We, a few months ago, introduced changes for a consistent interface of
`activeShape`

#3756

In doing so we created a regression when a bar `shape` is different than
the default activeBar of `true` (and therefore a regular Rectangle
component)

This causes components to change to Bar on hover - see
[sandbox](https://codesandbox.io/p/sandbox/mixed-bar-chart-forked-2wdtzj?file=%2Fpackage.json%3A11%2C21)

Also noticed TS freaking out in some of our tests. This.. isn't good and
means that the usability of some of these things is quite terrible ...

## 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: -->
#4103 and #4101

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
- Fix regression

## 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. -->
- Tests continue to pass besides the ones that should have broken
- Test by hand, everything else is the same

## Screenshots (if appropriate):

<img width="453" alt="image"
src="https://github.com/recharts/recharts/assets/25180830/2e27eeb8-5c38-4f5a-b05a-94f91ae60fb8">
Still works

Does not break custom shape 
<img width="207" alt="image"
src="https://github.com/recharts/recharts/assets/25180830/23f35287-db95-4157-9a76-c5971f0f1b76">


## 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.
- [X] 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.

Co-authored-by: Coltin Kifer <ckifer@amazon.com>
  • Loading branch information
ckifer and Coltin Kifer committed Feb 8, 2024
1 parent a929861 commit 723b843
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 32 deletions.
4 changes: 2 additions & 2 deletions src/cartesian/Bar.tsx
Expand Up @@ -95,7 +95,7 @@ export interface BarProps extends InternalBarProps {
label?: ImplicitLabelType;
}

export type Props = Omit<PresentationAttributesAdaptChildEvent<any, SVGPathElement>, 'radius'> & BarProps;
export type Props = Omit<PresentationAttributesAdaptChildEvent<any, SVGPathElement>, 'radius' | 'name'> & BarProps;

interface State {
readonly isAnimationFinished?: boolean;
Expand All @@ -115,7 +115,7 @@ export class Bar extends PureComponent<Props, State> {
hide: false,
data: [] as BarRectangleItem[],
layout: 'vertical',
activeBar: true,
activeBar: false,
isAnimationActive: !Global.isSsr,
animationBegin: 0,
animationDuration: 400,
Expand Down
2 changes: 1 addition & 1 deletion src/util/BarUtils.tsx
Expand Up @@ -9,7 +9,7 @@ import { Shape } from './ActiveShapeUtils';
// When props are being spread in from a user defined component in Bar,
// the prop types of an SVGElement have these typed as something else.
// This function will return the passed in props
// along with x, y, height as numbers, name as a string, and radius as number | [number, numbe, number, number]
// along with x, y, height as numbers, name as a string, and radius as number | [number, number, number, number]
function typeguardBarRectangleProps(
{ x: xProp, y: yProp, ...option }: SVGProps<SVGPathElement>,
props: BarProps,
Expand Down
1 change: 1 addition & 0 deletions storybook/stories/Examples/BarChart.stories.tsx
Expand Up @@ -187,6 +187,7 @@ export const CustomShape = {
>
<CartesianGrid strokeDasharray="3 3" />
<XAxis dataKey="name" />
<Tooltip />
<YAxis />
<Bar dataKey="uv" fill="#8884d8" shape={<TriangleBar />} label={{ position: 'top' }}>
{pageData.map(({ name }, index) => (
Expand Down
15 changes: 8 additions & 7 deletions test/chart/BarChart.spec.tsx
Expand Up @@ -2,7 +2,7 @@ import { fireEvent, render } from '@testing-library/react';
import React from 'react';

import { vi } from 'vitest';
import { Bar, BarChart, Rectangle, RectangleProps, Tooltip, XAxis, YAxis } from '../../src';
import { Bar, BarChart, BarProps, Rectangle, Tooltip, XAxis, YAxis } from '../../src';
import { assertNotNull } from '../helper/assertNotNull';
import { testChartLayoutContext } from '../util/context';

Expand Down Expand Up @@ -127,7 +127,7 @@ describe('<BarChart />', () => {
expect(container.querySelectorAll('.recharts-tooltip-wrapper')).toHaveLength(1);
});

test('Renders customized active bar by default', () => {
test('Does not render an active bar by default', () => {
vi.useFakeTimers();

const { container } = render(
Expand All @@ -146,7 +146,7 @@ describe('<BarChart />', () => {

vi.advanceTimersByTime(100);
const bar = container.querySelectorAll('.recharts-active-bar');
expect(bar).toHaveLength(1);
expect(bar).toHaveLength(0);
});

test('Renders customized active bar when activeBar set to be a function', () => {
Expand All @@ -157,8 +157,9 @@ describe('<BarChart />', () => {
dataKey="uv"
stackId="test"
fill="#ff7300"
activeBar={(props: RectangleProps) => {
return <Rectangle {...props} name={props.name as string} />;
activeBar={(props: BarProps) => {
// @ts-expect-error this should work but it doesn't because of the events injected into BarProps
return <Rectangle {...props} name={String(props.name)} />;
}}
/>
<Tooltip />
Expand Down Expand Up @@ -279,14 +280,14 @@ describe('<BarChart />', () => {
});

test('Render customized shapem when shape is set to be a function', () => {
const renderShape = (props: RectangleProps & DataType) => {
const renderShape = (props: BarProps): React.ReactElement => {
const { x, y } = props;

return <circle className="customized-shape" cx={x} cy={y} r={8} />;
};
const { container } = render(
<BarChart width={100} height={50} data={data}>
<Bar dataKey="uv" label fill="#ff7300" shape={renderShape} />
<Bar dataKey="uv" label fill="#ff7300" shape={(props: BarProps) => renderShape(props)} />
</BarChart>,
);
expect(container.querySelectorAll('.customized-shape')).toHaveLength(4);
Expand Down
44 changes: 22 additions & 22 deletions test/component/Tooltip.spec.tsx
Expand Up @@ -71,7 +71,7 @@ describe('<Tooltip />', () => {
);

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

// After the mouse over event over the chart, the tooltip wrapper still is not set to visible,
// but the content is already created based on the nearest data point.
Expand Down Expand Up @@ -99,10 +99,10 @@ describe('<Tooltip />', () => {
mock.mockRestore();

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseMove(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseMove(chart!, { clientX: 200, clientY: 200 });

const tooltip = container.querySelector('.recharts-tooltip-wrapper');
expect(tooltip.getAttribute('style').includes('translate')).toBe(true);
const tooltip = container.querySelector('.recharts-tooltip-wrapper')!;
expect(tooltip.getAttribute('style')!.includes('translate')).toBe(true);
});

test('Mouse over renders content with multiple data sets', () => {
Expand Down Expand Up @@ -138,7 +138,7 @@ describe('<Tooltip />', () => {
);

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

// After the mouse over event over the chart, the tooltip wrapper still is not set to visible,
// but the content is already created based on the nearest data point.
Expand Down Expand Up @@ -167,7 +167,7 @@ describe('<Tooltip />', () => {
{ category: 'D', value: 0.6 },
];

let tooltipPayload: any[] | null = null;
let tooltipPayload: any[] | null | undefined = null;

const { container } = render(
<div role="main" style={{ width: '400px', height: '400px' }}>
Expand All @@ -188,11 +188,11 @@ describe('<Tooltip />', () => {
);

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });
expect(tooltipPayload).not.toBeNull();
expect(tooltipPayload).toHaveLength(2);
expect(tooltipPayload[0].payload.value).toEqual(0.7);
expect(tooltipPayload[1].payload.value).toEqual(0.4);
expect(tooltipPayload![0]?.payload.value).toEqual(0.7);
expect(tooltipPayload![1]?.payload.value).toEqual(0.4);
});

it('Render customized tooltip when content is set to be a react element', () => {
Expand All @@ -208,7 +208,7 @@ describe('<Tooltip />', () => {

const chart = container.querySelector('.recharts-wrapper');

fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

const customizedContent = container.querySelector('.customized');
expect(customizedContent).toBeInTheDocument();
Expand All @@ -229,9 +229,9 @@ describe('<Tooltip />', () => {
</LineChart>,
);

const line = container.querySelector('.recharts-cartesian-grid-horizontal line');
const line = container.querySelector('.recharts-cartesian-grid-horizontal line')!;
const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: +line.getAttribute('x') + 1, clientY: 50 });
fireEvent.mouseOver(chart!, { clientX: +line.getAttribute('x')! + 1, clientY: 50 });
expect(getByText(container, '1398')).toBeVisible();
});
});
Expand All @@ -249,11 +249,11 @@ describe('<Tooltip />', () => {
expect(tooltip).not.toBeVisible();

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

expect(tooltip).toBeVisible();

fireEvent.mouseOut(chart);
fireEvent.mouseOut(chart!);

// Still visible after moving out of the chart, because active is true.
expect(tooltip).toBeVisible();
Expand All @@ -272,11 +272,11 @@ describe('<Tooltip />', () => {
expect(tooltip).not.toBeVisible();

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

expect(tooltip).not.toBeVisible();

fireEvent.mouseOut(chart);
fireEvent.mouseOut(chart!);

expect(tooltip).not.toBeVisible();
});
Expand All @@ -293,11 +293,11 @@ describe('<Tooltip />', () => {
expect(tooltip).not.toBeVisible();

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

expect(tooltip).toBeVisible();

fireEvent.mouseOut(chart);
fireEvent.mouseOut(chart!);

expect(tooltip).not.toBeVisible();
});
Expand Down Expand Up @@ -326,7 +326,7 @@ describe('<Tooltip />', () => {
expect(tooltipPayload).toHaveLength(0);

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

expect(tooltipPayload.map(({ name }) => name).join('')).toBe('12345');
});
Expand Down Expand Up @@ -354,7 +354,7 @@ describe('<Tooltip />', () => {
expect(tooltipPayload).toHaveLength(0);

const chart = container.querySelector('.recharts-wrapper');
fireEvent.mouseOver(chart, { clientX: 200, clientY: 200 });
fireEvent.mouseOver(chart!, { clientX: 200, clientY: 200 });

expect(tooltipPayload.map(({ name }) => name).join('')).toBe('5');
});
Expand Down Expand Up @@ -449,7 +449,7 @@ describe('<Tooltip />', () => {
test('defaultIndex should work with bar charts', () => {
const { container } = render(
<BarChart width={100} height={50} data={data}>
<Bar dataKey="uv" label fill="#ff7300" />
<Bar dataKey="uv" label fill="#ff7300" activeBar />
<Tooltip defaultIndex={2} />
</BarChart>,
);
Expand All @@ -463,7 +463,7 @@ describe('<Tooltip />', () => {
// The cursor should also be visible
expect(container.querySelector('.recharts-tooltip-cursor')).toBeVisible();

// The box around the active bar should also be visible
// The box around the active bar should also be visible (if set)
expect(container.querySelector('.recharts-active-bar')).toBeVisible();

// "2uv..." should be displayed in the Tooltip payload
Expand Down

0 comments on commit 723b843

Please sign in to comment.