Skip to content

CHARTS-176: Fix zero-value bars not visible in small chart heights#47477

Open
kangzj wants to merge 9 commits intotrunkfrom
fix/charts-176-zero-value-bars-small-height
Open

CHARTS-176: Fix zero-value bars not visible in small chart heights#47477
kangzj wants to merge 9 commits intotrunkfrom
fix/charts-176-zero-value-bars-small-height

Conversation

@kangzj
Copy link
Contributor

@kangzj kangzj commented Mar 6, 2026

Fixes https://linear.app/a8c/issue/CHARTS-176/mini-bar-for-zero-values-are-not-shown-when-height-is-small

Proposed changes:

  • Ensures bars remain visible in small chart heights by calculating minimum pixel-based values
  • Zero values render as 2px equivalent
  • Near-zero values (that would render < 3px) get boosted to 3px equivalent
  • This 1px difference ensures zeros are visually distinguishable from near-zero values

Implementation:

The useZeroValueDisplay hook now:

  1. Takes valueAxisLength (height for vertical charts, width for horizontal)
  2. Calculates the data value that corresponds to 3px for near-zero values
  3. Calculates the data value that corresponds to 2px for zero values
  4. Applies these as visualValue overrides for rendering

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Does this pull request change what data or activity we track or use?

No changes to data tracking.

Testing instructions:

  1. Run Storybook: pnpm --filter @automattic/charts storybook
  2. Navigate to Bar Chart → ZeroValueComparison story
  3. Verify:
    • Zero-value bars are visible in all chart sizes including the 100px height chart
    • Zero bars appear slightly smaller than near-zero value bars (1px difference)
  4. Hover over bars to confirm tooltips still show actual values

Alternatively, run the tests:

pnpm --filter @automattic/charts test
image

Changelog

  • Generate changelog entries for this PR (using AI).

Changelog entry already added manually.

When a chart has a small height, zero-value bars could become invisible
because the ratio-based calculation for minimum bar height doesn't account
for the actual pixel dimensions of the chart.

This fix:
- Adds a `chartHeight` parameter to `useZeroValueDisplay` hook
- Calculates a minimum visible value that ensures at least 3 pixels height
- Takes the maximum of ratio-based and pixel-based calculations
- Passes the appropriate dimension (height for vertical, width for horizontal)

Also adds comprehensive tests and a Storybook story demonstrating the fix.
Copilot AI review requested due to automatic review settings March 6, 2026 03:43
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the fix/charts-176-zero-value-bars-small-height branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack fix/charts-176-zero-value-bars-small-height

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Follow this PR Review Process:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 6, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses CHARTS-176 by ensuring “zero-value” bars remain visible when bar charts are rendered at small dimensions, by augmenting the zero-value substitution logic with a pixel-based minimum.

Changes:

  • Add an optional chartHeight option to useZeroValueDisplay and apply a pixel-based minimum (targeting a 3px bar).
  • Add unit tests for the hook’s new pixel-based behavior.
  • Add a BarChart regression test and a Storybook story demonstrating the small-height case.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
projects/js-packages/charts/src/hooks/use-zero-value-display.ts Adds chartHeight option and enforces a pixel-based minimum visible value.
projects/js-packages/charts/src/hooks/test/use-zero-value-display.test.tsx Introduces hook-level tests covering the new chartHeight behavior.
projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx Passes a chart dimension into useZeroValueDisplay for pixel-based scaling.
projects/js-packages/charts/src/charts/bar-chart/test/bar-chart.test.tsx Adds a regression test asserting visibility of zero bars in small chart heights.
projects/js-packages/charts/src/charts/bar-chart/stories/index.stories.tsx Adds a Storybook scenario to visually validate small-height zero bars.
projects/js-packages/charts/changelog/2026-03-06-03-42-25-547937 Patch changelog entry for the fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kangzj kangzj requested a review from a team March 6, 2026 03:48
@kangzj kangzj added Task [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 6, 2026
@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 6, 2026
@kangzj kangzj self-assigned this Mar 6, 2026
Copy link
Contributor Author

@kangzj kangzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed Copilot feedback:

  • Renamed chartHeight to valueAxisLength with updated JSDoc to clarify it represents the pixel length of the value axis (height for vertical, width for horizontal)
  • Added clamping to prevent minPixelBasedValue from exceeding maxAbsoluteValue when axis length is very small
  • Fixed test comment/assertion mismatch
  • Fixed formatting issues in test file

Note: Using prop dimensions instead of measured dimensions is intentional - the measured height is not available until after the first render, and using the prop value is conservative (provides a slightly larger minimum which ensures visibility).

Copilot AI review requested due to automatic review settings March 6, 2026 04:01
@kangzj kangzj removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 6, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

'Comparison showing the difference between disabled and enabled zero value display modes. The feature preserves data integrity by keeping the original value for tooltips while providing visual feedback through minimum bar heights.',
'Comparison showing the difference between disabled and enabled zero value display modes. The feature preserves data integrity by keeping the original value for tooltips while providing visual feedback through minimum bar heights. Zero-value bars remain visible even in small chart heights.',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show the whole story without scrolling

Suggested change
},
},
{
args: {
containerHeight: 1600
}
},

Copilot AI review requested due to automatic review settings March 6, 2026 04:09
Copy link
Contributor Author

@kangzj kangzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed new Copilot feedback:

  • Added clamping to prevent minNonZeroValue and zeroVisualValue from exceeding maxAbsoluteValue when axis length is very small
  • Hook is internal only (not exported from main package), so API change is not breaking
  • Test comments are accurate (mockData max=200, calculations match)

/**
* Minimum pixel size for near-zero bars (non-zero values that would render too small).
*/
const MIN_PIXEL_SIZE = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this value is a little high. 3px looks to me like there is some data. Would just 1px be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2026-03-06 at 5 14 41 PM

1px is a bit too little 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2px then 😂

Copy link
Contributor Author

@kangzj kangzj Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya it was 2px actually. 3px is the min height for non zero values.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* horizontal charts). Used to calculate a minimum visible value that ensures
* zero-value bars are at least MIN_PIXEL_HEIGHT pixels tall along that axis.
*/
valueAxisLength?: number;
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hook is part of the public @automattic/charts/hooks export (see projects/js-packages/charts/package.json), but UseZeroValueDisplayOptions removed the previously supported minValueRatio/maxValueRatio options. That’s a breaking change for any external consumers using those options; consider keeping them as deprecated no-ops / backwards-compatible aliases, or bumping appropriately and documenting the breaking change.

Suggested change
valueAxisLength?: number;
valueAxisLength?: number;
/**
* @deprecated This option is no longer used. It is retained as a no-op for
* backward compatibility with previous versions of this hook.
*/
minValueRatio?: number;
/**
* @deprecated This option is no longer used. It is retained as a no-op for
* backward compatibility with previous versions of this hook.
*/
maxValueRatio?: number;

Copilot uses AI. Check for mistakes.

// Should return original data since there are no non-zero values to calculate from
expect( result.current ).toBe( zeroOnlyData );
} );
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new hook tests don’t cover negative near-zero values or null values. Given BarChart supports negatives and DataPointDate.value is number | null, add cases to ensure boosted visualValue preserves sign and that null values remain untouched (no visualValue added).

Suggested change
} );
} );
test( 'negative near-zero values preserve sign when boosted', () => {
const data: SeriesData[] = [
{
label: 'Series 1',
data: [
{ label: 'NegativeNearZero', value: -1 }, // Would render as -1px
{ label: 'NegativeLarge', value: -100 },
{ label: 'PositiveMax', value: 100 },
],
},
];
const { result } = renderHook( () =>
useZeroValueDisplay( data, { enabled: true, valueAxisLength: 100 } )
);
const negativeNearZero = result.current[ 0 ].data[ 0 ] as { visualValue?: number };
// Should have a boosted magnitude but keep the negative sign (e.g., -3px equivalent)
expect( negativeNearZero.visualValue ).toBeLessThan( 0 );
expect( Math.abs( negativeNearZero.visualValue! ) ).toBeGreaterThan( 1 );
} );
test( 'null values remain untouched without visualValue', () => {
const data: SeriesData[] = [
{
label: 'Series 1',
data: [
{ label: 'NullValue', value: null },
{ label: 'NonZero', value: 50 },
],
},
];
const { result } = renderHook( () =>
useZeroValueDisplay( data, { enabled: true, valueAxisLength: 100 } )
);
const nullPoint = result.current[ 0 ].data[ 0 ] as { value: number | null; visualValue?: number };
// The original null value should be preserved and no visualValue should be added
expect( nullPoint.value ).toBeNull();
expect( 'visualValue' in nullPoint ).toBe( false );
} );

Copilot uses AI. Check for mistakes.
Comment on lines +74 to 82

const absValue = Math.abs( point.value ?? 0 );

// Near-zero values that would render below MIN_PIXEL_SIZE get boosted to 3px
if ( absValue < minNonZeroValue ) {
return {
...point,
visualValue: minNonZeroValue,
};
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataPointDate.value can be null, but the near-zero logic treats null as 0 via point.value ?? 0 and will add a visualValue, causing missing data to render as a small bar. Add an explicit early return for point.value === null (and keep nulls unmodified).

Copilot uses AI. Check for mistakes.
Comment on lines +566 to +582
test( 'ensures minimum pixel height for zero values in small charts', () => {
// With a small chart height (100px) and large data range, zero-value bars
// should still be visible (at least 3px based on MIN_PIXEL_HEIGHT)
renderWithTheme( {
showZeroValues: true,
height: 100,
data: [
{
label: 'Test Series',
data: [
{ label: 'Zero', value: 0 },
{ label: 'Large', value: 10000 },
],
options: {},
},
],
} );
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This “small charts” test isn’t actually exercising a 100px chart height because useElementSize is globally mocked to always return a wrapper height of 300 (see the mock at the top of this file). As a result, chartHeight stays 300 and the minimum-pixel logic isn’t being validated for small heights. Consider overriding the mock for this test (or making it configurable) so the measured height matches the intended small chart size.

Copilot uses AI. Check for mistakes.
Comment on lines +567 to +590
// With a small chart height (100px) and large data range, zero-value bars
// should still be visible (at least 3px based on MIN_PIXEL_HEIGHT)
renderWithTheme( {
showZeroValues: true,
height: 100,
data: [
{
label: 'Test Series',
data: [
{ label: 'Zero', value: 0 },
{ label: 'Large', value: 10000 },
],
options: {},
},
],
} );

const svgElement = screen.getByRole( 'grid', { name: /bar chart/i } ).querySelector( 'svg' );
const bars = svgElement?.querySelectorAll( '.visx-bar-group rect' );

expect( bars?.length ).toBe( 2 );

// The zero-value bar (first bar) should have a minimum visible height.
// We check for >= 2px to allow for rounding in the pixel calculation.
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says the zero-value bar should be visible “at least 3px”, but the current implementation renders zeros at 2px (and the assertion allows >= 2). Please update the comment to reflect the 2px/3px design (zeros 2px, near-zeros 3px) to avoid confusion.

Copilot uses AI. Check for mistakes.
Comment on lines +396 to +397
Zero-value bars remain visible even in small charts. The minimum pixel height ensures bars
are at least 3 pixels tall regardless of chart dimensions.
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The story text claims the “minimum pixel height ensures bars are at least 3 pixels tall”, but the implementation intentionally renders zero values at 2px (1px less than near-zero). Please update this copy to match the 2px/3px behavior so Storybook guidance is accurate.

Suggested change
Zero-value bars remain visible even in small charts. The minimum pixel height ensures bars
are at least 3 pixels tall regardless of chart dimensions.
Zero-value bars remain visible even in small charts. Zero values are rendered at 2 pixels
tall, while non-zero values use a minimum height of 3 pixels to stay visible regardless of
chart dimensions.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +81
const absValue = Math.abs( point.value ?? 0 );

// Near-zero values that would render below MIN_PIXEL_SIZE get boosted to 3px
if ( absValue < minNonZeroValue ) {
return {
...point,
visualValue: minNonZeroValue,
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Near-zero boosting sets visualValue to a positive minNonZeroValue regardless of the original sign. For negative values (supported by BarChart), this will flip small negative bars to positive and render them on the wrong side of the axis. Preserve the sign when assigning visualValue (e.g., apply Math.sign(point.value) when point.value is non-zero).

Suggested change
const absValue = Math.abs( point.value ?? 0 );
// Near-zero values that would render below MIN_PIXEL_SIZE get boosted to 3px
if ( absValue < minNonZeroValue ) {
return {
...point,
visualValue: minNonZeroValue,
// Preserve null values as-is; they should not be treated as near-zero numbers.
if ( point.value == null ) {
return point;
}
const absValue = Math.abs( point.value );
// Near-zero values that would render below MIN_PIXEL_SIZE get boosted to 3px
if ( absValue < minNonZeroValue ) {
const sign = Math.sign( point.value );
const signedMinNonZeroValue =
sign === 0 ? minNonZeroValue : sign * minNonZeroValue;
return {
...point,
visualValue: signedMinNonZeroValue,

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach LGTM, and it works. Left a couple of comments.

- Reduce MIN_PIXEL_SIZE from 3px to 2px (near-zero values) and ZERO_PIXEL_SIZE from 2px to 1px (zero values) to avoid misleading users into thinking there's actual data
- Add docs.story.height to show the ZeroValueComparison story without scrolling
- Rename changelog to descriptive name: charts-176-fix-zero-value-display-small-charts
- Update tests and story descriptions to reflect new pixel values
@jp-launch-control
Copy link

Code Coverage Summary

Coverage changed in 1 file.

File Coverage Δ% Δ Uncovered
projects/js-packages/charts/src/hooks/use-zero-value-display.ts 21/21 (100.00%) 0.00% 0 💚

Full summary · PHP report · JS report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants