Skip to content

Commit

Permalink
fix(CartesianAxis): render the axis line when there are no ticks (#4433)
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 -->
Bug reported in linked issue where after recharts 2.5 the YAxis line
would "disappear" with 0 data points or ticks, where it would previously
show.

It would previously show because `NaN` was being calculated as a tick
value and added to the dom as a tick item. This was fixed in
#3454.

I would not call this a regression because as per the lines changed in
this PR, the axis lines were never meant to show unless there were
ticks. (See `!finalTicks || !finalTicks.length`). I believe however,
that they should be able to.

Thus, this is a breaking change for both X and Y axes where even with 0
ticks or 0 valid data items, the axis lines will still render.

For a user to return to the old behavior of no Axis, they only have to
do something simple like `YAxis hide={data.length === 0} />` or use
React convention and conditionally render it

## 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: -->
#4426

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
- It makes sense to show an axis line even when there are no ticks

## 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. -->
- unit tests
- see screenshot

## Screenshots (if appropriate):
<img width="864" alt="image"
src="https://github.com/recharts/recharts/assets/25180830/c10295ea-a30c-481c-b313-2ca4936dc3d4">

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

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

Co-authored-by: Coltin Kifer <ckifer@amazon.com>
  • Loading branch information
ckifer and Coltin Kifer committed Apr 18, 2024
1 parent 49f8c0f commit 1d60cc6
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 6 deletions.
8 changes: 4 additions & 4 deletions src/cartesian/CartesianAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,9 @@ export class CartesianAxis extends Component<Props, IState> {
* @param {Array} ticks The ticks to actually render (overrides what was passed in props)
* @param {string} fontSize Fontsize to consider for tick spacing
* @param {string} letterSpacing Letterspacing to consider for tick spacing
* @return {ReactComponent} renderedTicks
* @return {ReactElement | null} renderedTicks
*/
renderTicks(ticks: CartesianTickItem[], fontSize: string, letterSpacing: string) {
renderTicks(ticks: CartesianTickItem[] = [], fontSize: string, letterSpacing: string): React.ReactElement | null {
const { tickLine, stroke, tick, tickFormatter, unit } = this.props;
const finalTicks = getTicks({ ...this.props, ticks }, fontSize, letterSpacing);
const textAnchor = this.getTickTextAnchor();
Expand Down Expand Up @@ -313,7 +313,7 @@ export class CartesianAxis extends Component<Props, IState> {
);
});

return <g className="recharts-cartesian-axis-ticks">{items}</g>;
return items.length > 0 ? <g className="recharts-cartesian-axis-ticks">{items}</g> : null;
}

render() {
Expand All @@ -330,7 +330,7 @@ export class CartesianAxis extends Component<Props, IState> {
finalTicks = ticks && ticks.length > 0 ? ticksGenerator(this.props) : ticksGenerator(noTicksProps);
}

if (width <= 0 || height <= 0 || !finalTicks || !finalTicks.length) {
if (width <= 0 || height <= 0) {
return null;
}

Expand Down
1 change: 0 additions & 1 deletion src/cartesian/YAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export const YAxis: FunctionComponent<Props> = ({ yAxisId }: Props) => {
const width = useChartWidth();
const height = useChartHeight();
const axisOptions = useYAxisOrThrow(yAxisId);

if (axisOptions == null) {
return null;
}
Expand Down
15 changes: 15 additions & 0 deletions test/cartesian/XAxis.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,21 @@ describe('<XAxis />', () => {
expect(bar).not.toBeInTheDocument();
});

it('Should render the YAxis line without any ticks', () => {
const barData = [{ day: '05-01' }, { day: '05-02' }];
const { container } = render(
<BarChart width={300} height={300} data={barData}>
<Bar dataKey="y" isAnimationActive={false} />
<XAxis dataKey="y" type="number" />
</BarChart>,
);
const ticksGroup = container.getElementsByClassName('recharts-cartesian-axis-tick-line');
expect(ticksGroup).toHaveLength(0);

const axisLine = container.getElementsByClassName('recharts-cartesian-axis-line');
expect(axisLine).toHaveLength(1);
});

it('Render Bars for a single data point with barSize=50%', () => {
const { container } = render(
<BarChart width={300} height={300} data={data.slice(0, 1)} barSize="50%">
Expand Down
17 changes: 16 additions & 1 deletion test/cartesian/YAxis.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ describe('<YAxis />', () => {
expect(ticks[1].getAttribute('y')).toBe('102.5');
});

it("Don't render empty tick", () => {
it('Should skip rendering ticks with empty text', () => {
const areaData = [
{ day: '05-01', weather: 'sunny' },
{ day: '05-02' },
Expand All @@ -170,6 +170,21 @@ describe('<YAxis />', () => {
expect(ticks).toHaveLength(3);
});

it('Should render the YAxis line without any ticks', () => {
const areaData = [{ day: '05-01' }, { day: '05-02' }];
const { container } = render(
<AreaChart width={400} height={400} data={areaData}>
<YAxis type="category" />
<Area type="stepAfter" dataKey="weather" stroke="#0088FE" />
</AreaChart>,
);
const ticksGroup = container.getElementsByClassName('recharts-cartesian-axis-tick-line');
expect(ticksGroup).toHaveLength(0);

const axisLine = container.getElementsByClassName('recharts-cartesian-axis-line');
expect(axisLine).toHaveLength(1);
});

it('should throw when attempting to render outside of Chart', () => {
expect(() => render(<YAxis dataKey="x" name="stature" unit="cm" />)).toThrow(
'Invariant failed: Could not find Recharts context; are you sure this is rendered inside a Recharts wrapper component?',
Expand Down

0 comments on commit 1d60cc6

Please sign in to comment.