Fix Axis blinking when changing props, and YAxis width=auto wrapping#6262
Fix Axis blinking when changing props, and YAxis width=auto wrapping#6262PavelVanecek merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a blinking issue in XAxis and YAxis components that occurred on weak hardware in Chrome by removing early returns that caused components to disappear during prop synchronization. The fix ensures axis components use synchronized settings from the Redux store rather than component props to prevent render inconsistencies.
Key changes:
- Removes conditional returns that caused axis blinking during prop changes
- Updates axis components to use synchronized settings from the store
- Adjusts test expectations to reflect the additional render call
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/cartesian/XAxis.tsx | Removes early return logic and adds synchronized settings usage to prevent blinking |
| src/cartesian/YAxis.tsx | Adds synchronized settings usage and updates null check condition |
| test/chart/LineChart.spec.tsx | Updates test expectations from 3 to 4 axis render calls |
| test/chart/AreaChart.spec.tsx | Updates test expectations from 3 to 4 axis render calls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6262 +/- ##
=======================================
Coverage 96.62% 96.63%
=======================================
Files 221 221
Lines 20182 20195 +13
Branches 4140 4145 +5
=======================================
+ Hits 19501 19515 +14
+ Misses 674 673 -1
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 3.9kB (0.16%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
|
Huh, that's pretty wild |
|
There's something wrong with Text wrapping. Let's wait with merging this before I figure it out. |
|
Sounds good |
## Description Just tests, no changes. ## Related Issue I want to have baseline to debug the regression from #6262
9211ca5 to
2df4a5b
Compare
| if (settingsAreSynchronized) { | ||
| return props.children; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
This was the "blink"
| scale={scale} | ||
| x={position.x} | ||
| y={position.y} | ||
| tickTextProps={width === 'auto' ? { width: undefined } : { width }} |
There was a problem hiding this comment.
If width is auto then we need to let Text calculate its own width, so we overwrite whatever width is coming from the axisProps
There was a problem hiding this comment.
We have some visual diff in the PR.
This source code is:
<YAxis width="auto" tickFormatter={value => `Long Long Y tick: ${value}`} />I think it's an improvement - I would expect width=auto to show ticks unwrapped. Or should we prefer one word per line?
There was a problem hiding this comment.
I think unwrapped by default but there were some people talking about this I think. It makes width auto hard to use when you have long ticks
There was a problem hiding this comment.
<YAxis width="auto" tick={{ fontSize: 9 }} tickFormatter={value => `Long Long Y tick: ${value}`} />Same as above, I consider this an improvement
|
What still doesn't work is |
|
It doesn't work pre this either? |
|
No it ignores the prop always. Before this PR it always wraps, after it never wraps. (With width auto) |
Description
Remove the return null that makes the XAxis disappear in Chrome on weak hardware (or 4x CPU slowdown)
More findings
Okay so once I fixed the blink, the text started wrapping with width=auto. I can fix that by excluding the axis width from tick props if width=auto (because in that case we want the Text to calculate its own width, and push it up to the axis). That creates visual diff elsewhere but that looks more like a bugfix to me so I will do that.
Related Issue
Fixes #6257