Skip to content
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: [React 19] Fix BarChart Rendering Issue with XAxis Height #1075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

soyricardodev
Copy link

Description

This PR fixes a bug where the height prop of the XAxis component in BarChart.tsx could result in a NaN value, causing a warning message ("Received NaN for the height attribute") and don't rendering bar charts in react 19.

Problem:
The original code (height={rotateLabelX?.xAxisHeight}) was susceptible to returning NaN in certain scenarios.

Solution
This PR introduces a default value (0) using the nullish coalescing operator (??). This ensures a valid height is always set, preventing the warning and potentially improving rendering of BarCharts.
Related issue(s)
tremorlabs/tremor#1054

What kind of change does this PR introduce? (check at least one)

  • Bug fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • New Feature (BREAKING CHANGE which adds functionality)
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

How has this been tested?
I've tested this fix in a personal project using Tremor components locally. You can find the project repository here: soyricardodev/theliaison

The relevant code changes can be found in the src/components/tremor directory. While the code is not fully polished or organized, it demonstrates the approach I took to resolve the issue in my local environment and highlights the simplicity of the fix.

Screenshots (if appropriate):

The PR fulfils these requirements:

  • It's submitted to the main branch
  • When resolving a specific issue, it's referenced in the related issue section above
  • My change requires a change to the documentation. (Managed by Tremor Team)
  • I have added tests to cover my changes
  • Check the "Allow edits from maintainers" option while creating your PR.
  • Add refs #XXX or fixes #XXX to the related issue section if your PR refers to or fixes an issue.
  • By contributing to Tremor, you confirm that you have read and agreed to Tremor's CONTRIBUTING.md guideline. You also agree that your contributions will be licensed under the Apache License 2.0 license.

Copy link

vercel bot commented Jun 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tremor-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:48am

@soyricardodev soyricardodev changed the title [React 19] Fix BarChart Rendering Issue with XAxis Height fix: [React 19] Fix BarChart Rendering Issue with XAxis Height Jun 3, 2024
@izakfilmalter
Copy link

This helps the graph render, but the x axis labels are hidden now by default.

@soyricardodev
Copy link
Author

This helps the graph render, but the x axis labels are hidden now by default.

:0 oh can you tell me more about that? i see that you have an issue #4586

@severinlandolt
Copy link
Member

Hi @soyricardodev, thanks for reporting. You wrote: "... susceptible to returning NaN in certain scenarios". Can you elaborate what in wich scenarios you got this error? Thanks!

@severinlandolt severinlandolt added the Status: Need more info The issue still hasn't been fully clarified label Jun 22, 2024
@soyricardodev
Copy link
Author

Hi @severinlandolt , thanks for answer. The scenario is React 19, here is a repo with react 19 and next 15 where you can debug the error and see: Repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Need more info The issue still hasn't been fully clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants