-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[core] Improve React 19 support #16048
Conversation
Signed-off-by: Armin Mehinovic <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jose Quintas <[email protected]> Co-authored-by: Lukas <[email protected]> Co-authored-by: flavien <[email protected]> Co-authored-by: Armin Mehinovic <[email protected]> Co-authored-by: Armin Mehinovic <[email protected]> Co-authored-by: Bilal Shafi <[email protected]>
Deploy preview: https://deploy-preview-16048--material-ui-x.netlify.app/ |
@@ -12,7 +12,7 @@ export function ItemTooltipFixedY() { | |||
const svgRef = useSvgRef(); // Get the ref of the <svg/> component. | |||
const drawingArea = useDrawingArea(); // Get the dimensions of the chart inside the <svg/>. | |||
|
|||
if (!tooltipData || !mousePosition) { | |||
if (!tooltipData || !mousePosition || !svgRef.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unique to v7.
@@ -29,7 +29,8 @@ export function ItemTooltipTopElement() { | |||
if ( | |||
tooltipData.identifier.type !== 'bar' || | |||
tooltipData.identifier.dataIndex === undefined || | |||
tooltipData.value === null | |||
tooltipData.value === null || | |||
svgRef.current === null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unique to v7.
return undefined; | ||
} | ||
const element = svgRef.current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically replicated the situation in master
to avoid the element
being | null
below.
skipAnimation | ||
slots={slots} | ||
slotProps={slotProps} | ||
sx={{ shapeRendering: 'auto' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works on v7, but is once again an unknown prop. 🤔 🙈
How do you want to handle it for v7 @mui/xcharts?
Reference for v8/master: https://github.com/mui/mui-x/pull/15769/files#r1892161016
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JCQuintas WDYT about this case?
For v7 this is a tiny behavior change as sx
works. 🤔
Maybe we should just ignore this line on v7 to avoid unwanted changes. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove is ok, doesn't affect visual it seems
@@ -7,7 +7,7 @@ import { Initializable } from './context.types'; | |||
|
|||
export interface DrawingProviderProps extends LayoutConfig { | |||
children: React.ReactNode; | |||
svgRef: React.RefObject<SVGSVGElement>; | |||
svgRef: React.RefObject<SVGSVGElement | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are unique to v7.
@LukasTy charts are not working on the preview. I guess some of the changes broke it. I'll eventually check it this week 😆 |
Why argos has a lot of changes and additions? 🤔 |
It might have been a flaky run/diff on Argos side. 🤔 |
@JCQuintas it could have been produced by the run on react 18.3.1. 🙈 |
Cherry-pick #15769.
@mui/internal-test-utils
@testing-library/react
,sinon
,jsdom
,mocha