Skip to content

Replace deprecated useResizeObserver for useScaleCanvas #67508

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.

Sign up for GitHub

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

Open
ajlende wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Replace deprecated useResizeObserver for useScaleCanvas #67508

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
61aa761
Remove deprecated usage of useResizeObserver
ajlende Dec 3, 2024
aa571a9
Use useState so resizes re-render
ajlende Dec 3, 2024
a9d3482
Add types and details in JSDoc comments
ajlende Dec 3, 2024
d00dc06
Fix variable name typo
ajlende Dec 3, 2024
ef60a79
Rename ref callback suffixes
ajlende Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Remove deprecated usage of useResizeObserver
  • Loading branch information
@ajlende
ajlende committed Dec 4, 2024
commit 61aa761906611ded733d2e3c125f625b869ef00a
12 changes: 7 additions & 5 deletions packages/block-editor/src/components/iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ function Iframe( {
}, [] );

const {
contentResizeListener,
containerResizeListener,
contentRefCallback,
containerRefCallback,
isZoomedOut,
scaleContainerWidth,
} = useScaleCanvas( {
Expand All @@ -234,6 +234,7 @@ function Iframe( {
clearerRef,
writingFlowRef,
disabledRef,
contentRefCallback,
] );

// Correct doctype is required to enable rendering in standards
Expand Down Expand Up @@ -341,7 +342,6 @@ function Iframe( {
...bodyClasses
) }
>
{ contentResizeListener }
<StyleProvider document={ iframeDocument }>
{ children }
</StyleProvider>
Expand All @@ -354,8 +354,10 @@ function Iframe( {
);

return (
<div className="block-editor-iframe__container">
{ containerResizeListener }
<div
className="block-editor-iframe__container"
ref={ containerRefCallback }
>
<div
className={ clsx(
'block-editor-iframe__scale-container',
Expand Down
50 changes: 29 additions & 21 deletions packages/block-editor/src/components/iframe/use-scale-canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ function getAnimationKeyframes( transitionFrom, transitionTo ) {
];
}

function extractSize( entries ) {
const contentBlockSize = entries.at( -1 ).contentBoxSize[ 0 ];
return {
width: contentBlockSize.inlineSize,
height: contentBlockSize.blockSize,
};
}

/**
* @typedef {Object} ScaleCanvasResult
* @property {boolean} isZoomedOut A boolean indicating if the canvas is zoomed out.
Expand All @@ -157,12 +165,14 @@ export function useScaleCanvas( {
maxContainerWidth = 750,
scale,
} ) {
const [ contentResizeListener, { height: contentHeight } ] =
useResizeObserver();
const [
containerResizeListener,
{ width: containerWidth, height: containerHeight },
] = useResizeObserver();
const contentRectRef = useRef( { width: null, height: null } );
const contentRefCallback = useResizeObserver( ( entries ) => {
contentRectRef.current = extractSize( entries );
} );
const containerRectRef = useRef( { width: null, height: null } );
const containerRefCallback = useResizeObserver( ( entries ) => {
containerRectRef.current = extractSize( entries );
} );

const initialContainerWidthRef = useRef( 0 );
const isZoomedOut = scale !== 1;
Expand All @@ -176,19 +186,19 @@ export function useScaleCanvas( {

useEffect( () => {
if ( ! isZoomedOut ) {
initialContainerWidthRef.current = containerWidth;
initialContainerWidthRef.current = containerRectRef.current.width;
}
}, [ containerWidth, isZoomedOut ] );
}, [ isZoomedOut ] );

const scaleContainerWidth = Math.max(
initialContainerWidthRef.current,
containerWidth
containerRectRef.current.width
);

const scaleValue = isAutoScaled
? calculateScale( {
frameSize,
containerWidth,
containerWidth: containerRectRef.current.width,
maxContainerWidth,
scaleContainerWidth,
} )
Expand Down Expand Up @@ -354,9 +364,9 @@ export function useScaleCanvas( {
// exiting.
transitionFromRef.current.scaleValue = calculateScale( {
frameSize: transitionFromRef.current.frameSize,
containerWidth,
containerWidth: containerRectRef.current.width,
maxContainerWidth,
scaleContainerWidth: containerWidth,
scaleContainerWidth: containerRectRef.current.width,
} );
}

Expand All @@ -378,17 +388,17 @@ export function useScaleCanvas( {

iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-content-height',
`${ contentHeight }px`
`${ contentRectRef.current.height }px`
);

iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-inner-height',
`${ containerHeight }px`
`${ containerRectRef.current.height }px`
);

iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-container-width',
`${ containerWidth }px`
`${ containerRectRef.current.width }px`
);
iframeDocument.documentElement.style.setProperty(
'--wp-block-editor-iframe-zoom-out-scale-container-width',
Expand Down Expand Up @@ -438,7 +448,8 @@ export function useScaleCanvas( {
transitionFromRef.current.scrollHeight =
iframeDocument.documentElement.scrollHeight;
// Use containerHeight, as it's the previous container height before the zoom out animation starts.
transitionFromRef.current.containerHeight = containerHeight;
transitionFromRef.current.containerHeight =
containerRectRef.current.height;

transitionToRef.current = {
scaleValue,
Expand Down Expand Up @@ -474,17 +485,14 @@ export function useScaleCanvas( {
scaleValue,
frameSize,
iframeDocument,
contentHeight,
containerWidth,
containerHeight,
maxContainerWidth,
scaleContainerWidth,
] );

return {
isZoomedOut,
scaleContainerWidth,
contentResizeListener,
containerResizeListener,
contentRefCallback,
containerRefCallback,
Copy link
Member

@jsnajdr jsnajdr Dec 3, 2024

Choose a reason for hiding this comment

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

Let's call it just contentRef/containerRef. The fact that it's a callback is an implementation detail that nobody really needs to know.

Also I think React Compiler treats variables differently when their name has the *Ref suffix.

Copy link
Contributor Author

@ajlende ajlende Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's call it just contentRef/containerRef. The fact that it's a callback is an implementation detail that nobody really needs to know.

When I'm writing my own React code, I typically differentiate between RefObject<T> and RefCallback<T> with different suffixes so I know if I can access a .current property or not.

Also I think React Compiler treats variables differently when their name has the *Ref suffix.

Thanks for bringing that to my attention. It looks like React Compiler bases it on the useRef() call, not the name of the variable.

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAgHgBwjALgAgBMEAzAQygBsCSoA7OXASwjvyjAQFEA3BO3AAo4ZSpQBGZOAGsAlPmAAdNvjiswBGKXwBedpwBKpQYPk6AfAuX4b+XAAsYEAO746CV1xhOYg67ZsAcgBhUQkpaTBVLTJcBEJ8ZyYHfAADDm4+AVTVMjo6CAJxBFyxeKJYJjoAc3wtOmIYKuqAOkD-G1kAbn8AX27-DIBJOk48FjouEhIERhMzSyUVGy0SFrhYeoI9ETFJGR6VfsOVhFxYNgzQvYi-ZfxBFqeyGGqwBbrSdc3+XAB+FqPZ6vd4AGg6+AA2gBdfwDOi9ZTKDDYPBEUgUaj4WgMZisfAAWQAngBBTCYUxWFRqUYEAC2RKMJF0+kyv3mukW+GO-hpGnwDKZ13CMhZGV47MpFgU3Nk-i05xgbDuAQAPOYITZVYQmDxPiQdMBBaRevgAPQa+5anV61aG40kYX7aSmi0Q1VulTwxF0EC9IA

I know ESLint's react/compiler rules are still a little wonky. Maybe ESLint was using the *Ref suffix for checking before, but I don't see any errors now. 🤷

That all being said, I don't see anyone else differentiating them in Gutenberg though, so I suppose I can change it here.

Copy link
Member

@jsnajdr jsnajdr Dec 4, 2024

Choose a reason for hiding this comment

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

It looks like React Compiler bases it on the useRef() call

@tyxla had to change a lot of ref variable names in #64718 to prevent a "mutating a value" error from the compiler. So the names matter.

In this case contentRefCallback would probably still be fine, because there is never any .current = ... assignment related to contentRefCallback.

Copy link
Member

@tyxla tyxla Dec 4, 2024

Choose a reason for hiding this comment

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

The ESLint plugin has been enabled since #61788, so it should automatically detect if we have a problem with the ref and if it needs to be named with *Ref. If it didn't detect it, then you're most likely not mutating the ref's .current, which is totally fine.

Copy link
Contributor Author

@ajlende ajlende Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did some more testing in the React Compiler Playground. I didn't have the enableTreatRefLikeIdentifiersAsRefs option set which corresponds to the ESLint rule that you're talking about. The new example has it enabled.

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAejQAgAIIHYCGARgDYIAqMCBALgEoIBmAMgJYDWCAkgCb42tGrBDDABBMA0ZhMAXkw0YUBAG4AOng0Y5s3Xv0HDR4ycNasAOQIBbBDIiNMVAOZQSBGJihgEUp0xkeCDs8AHIaTB8EawUITGtaGhEAOnMdUwzMrP0NDUYoPDgBCDxMAFkATykACgBKTGANTEw4ErAI6yqmOS8fGrw3Elr1Ut6EAFFGRgQi6rq5AD4Gpub4rsZkuFgqPAj5AZISEeaAX2GVqhpYUoAeHlYAN39GWWBOqRPMNAWRk9y8fKFYqlSoAeSIACsZjR5o1Rq08O01uCoUUet5fExqgchscxpNprN5rIlnDVl80CtmtoAGIEEg+TAABwgYFYAgeCCpFO52nGAGU2LtkOUoDRaKw8M5MARMA96cp-FcYHgEDxMIwYBAYrLAUVWCVMAB3AAWrIQSuucoVFrAZrc6rwEAiRAt1jFtDVqVGzU6KOhm22-B6OLxZzxlytd0ez1efsh0M+31+-202XTGaMaQYytV6qo0hlO3CMsOECNatimFd8Q9SR43rTmebGf+euBYxz1xqkvZrHp9TJFJabQiBfRfSxvYEA7x2kjKueEYQuc7U7wfdnGj+mgBBX1hsqXZVUhqg5WCKR7xX1zVfnkGOPeH6g3OowxBOhc3qJOWPrWT53kwgYwDseyYKGKzhhcN6LtGTwFnGXS5kBjBJj825tvuHZHrBz5MP6sznvCo4AXhaqEeBj54S+hxvs0H5TF+xKkryWACkKNAimUdaStKsryiQioLnmGpajqGrYQapSmualqLoJip2tAJCOs61ZunWXpsZgXB4IJrA8AwBBFNxvFSjK1pCRaImVpq2qWe20nGmajIiVZSn2qpEHqTW7rivW3rkto14oTwlEgWBIaDGG9Hybc9wIUwSGAeFCZFOhKa7k2La5Rk2bkeqcD0iQRAmewMgmgQnI+S0Qa7MyWpMiINAVFWJmIGAMgEHgFRGgQFSNlgeUjSYWFAs5H6crs1TFYcZVwOwxHNJeY7dA+k6MN+iwNJg0Hvj4ek+DAwKfkSP6sf+BaRcG8hzaV5Wxcuq4YgAwiVC3sHMyQ-R4zhgBdzw3bsAD8yTVD9yR-QDAA0mAANoALpvjueRSYeFTjNN9BYstI6Ih0mPY-e+LY9tv7AHtcWMYSMIsX+wWUv+tL0oyqrOBKnJsdynRY-wUjA+BkGjPtzTufBsZvET-NMJlmG7k5GN87s73zeVsIXqRvPY6rD2LROExk-TlOi-iTHnTtQ7UuxgqSlxooBXxlmKTZhViQ5urozJrmu6uLuRF5akuppAXaczWB6QZRnUKZDsShZAk2vFdniY5XsuXJ7n+8pDo+cHtahw2Ona-wuufYL0WHE9MGrhLiFS8rNBl+Vct4KjeA5aNXe6GkACqbIJ4cCgmharTWCyqq7DIkDDwgbUeBaJX+CZVWkAgQ3pN3Xf-AgAAeLInZgfCMAQbgRIrIIVGITJMhrozudU3I3As3LNDcuGON8r+YO-FSUV8L9-xv1wrmU83Qv5AJ-iA7sBF0oRAgeSYB0tdh+AQYg3+jdm76zQW-NBKMNAgBOEAA

I'm wondering if we should actually have a different name for RefCallback<T> variables because they don't have a .current property like the RefObject<T> variables? A Ref suffix makes the ESLint (and React Compiler) think that they do with the enableTreatRefLikeIdentifiersAsRefs option enabled.

This PR probably isn't the best place to discuss, so I'll open a discussion and link it here.

EDIT: Discussion in #67756

};
}