这是indexloc提供的服务,不要输入任何密码
Skip to content

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.

@vercel
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 Sep 19, 2024 11:23pm

@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

@severinlandolt severinlandolt changed the base branch from main to beta-tailwind-merge-update September 19, 2024 23:20
@severinlandolt severinlandolt added Status: In Progress We are actively fixing/implementing this issue/feature and removed Status: Need more info The issue still hasn't been fully clarified labels Sep 19, 2024
@severinlandolt severinlandolt changed the base branch from beta-tailwind-merge-update to main September 19, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: In Progress We are actively fixing/implementing this issue/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants