[RNMobile] Create cross platform useResizeObserver() hook#19918
[RNMobile] Create cross platform useResizeObserver() hook#19918lukewalczak merged 12 commits intomasterfrom
useResizeObserver() hook#19918Conversation
|
I realize this is still draft, but worth noting: The pattern of using higher-order components is generally discouraged in this project nowadays with the advent of React hooks typically being able to serve the same requirements in a more expressive manner. If there's an equivalent way to express this via introduction of a new hook, I would encourage that exploration instead. |
|
Hello @aduth , that sounds reasonable, so I can rewrite it to hooks. However, for the class component, I would like to create a HOC layer which will use new hook underneath. What do you think about that? |
I don't have enough of the context to understand how this is intended to be used, but: Broadly speaking, my observation is that function components with hooks have become the de facto preferred approach for all new component code such that we shouldn't ever need class components. I would sooner prefer to refactor any class components which would use this behavior, than to introduce a new higher-order component to support them. We have a number of existing higher-order components which are implemented like what you suggest ( Related Slack discussion from earlier last year (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1557239626309000 There might be an argument to make that, since the precedent already exists, we should offer higher-order component forms of every hook, largely as a "consistency for consistency's sake" in avoiding potential confusions surrounding the availability of some (but not all) higher-order components. For this, I think it would be something where we could try to deemphasize the availability of those higher-order components such that the documentation is only relevant for legacy purposes. In the past, I have also observed a few instances where higher-order components can provide advantages (example), but I don't think they're compelling enough to sway me too much. cc @youknowriad (in case you have thoughts) |
I'd also encourage this path forward as well. This PR makes me wonder if this is just the native alternative for useViewportMatch? Or maybe this is more about specific elements and not the entire viewport. |
|
Basically, functionality of that HOC is really similar to |
|
I rewrote a functionality to hook called |
withContainerMatches HOCuseContainerMatch hook
useContainerMatch hookuseContainerMatch hook
|
I was wondering if there's a way to have the exact same API as the web version useViewportMatch with a custom native implementation? |
|
Actually it's. My first attempt was based on breakpoints but specifically customized for mobile requirements (different values, different names etc). However, the idea of passing the custom width sounds better for me. |
It appears your message may have been partially cut off, so apologies if I'm misinterpreting, but: To me, if the goals of the hooks are largely overlapping (i.e. match and re-render in response to viewport size), we should see if we can adapt the existing one to fit the particular implementation needs. I don't see it being overly problematic to add functionality to |
|
On mobile platforms, I don't need / can't rely on viewport dimensions since these are quite fixed (dimensions are changing only on phone rotation). In that case, I need more to measure the parent (container) dimensions, more specifically how its width is changing, e.g in |
|
Ah! Is the requirement more like what was explored in #18745 where I could see how that's serving a different purpose than just viewport dimensions. And I suppose For example: // DOM:
const [ matches, resizeListenerElement ] = useContainerMatch( /* ... */ );
return <div>{ resizeListenerElement }<MyContent /></div>;
// Native:
const [ matches, onLayoutCallback ] = useContainerMatch( /* ... */ );
return <View onLayout={ onLayoutCallback }><MyContent /></View>;Do these ideas make sense? Or are these requirements not as aligned as I'm thinking them to be? |
|
Your comparison example presented it correctly. Exactly, my mobile hook is really similar to |
0a1691f to
64f894a
Compare
It makes a lot of sense to me, and I'd like to come up with not just a similar, but an identical API. I've been keeping an eye on the ResizeObserver API, which is gaining adoption, so maybe we can borrow that terminology. There's even a The one thing that's bugging me is that I think that library provides a cleaner API for the web, using refs instead of having to insert a child component, but I don't think we can have the same on native, since the only way to get the dimensions is via an So I think a mix of both approaches could work well: const { resizeObserver, width, height } = useResizeObserver();
return <View>{ resizeObserver }<MyContent /></View>;For the web, we can continue using For native, we can use a similar approach to what |
|
I forgot to mention, because this PR was actually implementing |
useContainerMatch hookuseResizeObserver() hook
aduth
left a comment
There was a problem hiding this comment.
I like how this is coming together 👍
|
@aduth Thanks for the valuable feedback! I hope that correctly resolved your suggestions. |
| class="components-placeholder wp-block-embed" | ||
| class="components-placeholder wp-block-embed is-large" | ||
| > | ||
| <iframe | ||
| aria-hidden="true" | ||
| aria-label="resize-listener" | ||
| frameborder="0" | ||
| src="about:blank" | ||
| style="display:block;opacity:0;position:absolute;top:0;left:0;height:100%;width:100%;overflow:hidden;pointer-events:none;z-index:-1" | ||
| tabindex="-1" | ||
| /> |
There was a problem hiding this comment.
These snapshot changes do not seem like they are sensible, since they undo the fixes intended with #19825.
There was a problem hiding this comment.
Oh, I see that you updated the mocking in the tests, so that a value is provided. So maybe they are sensible. I'll take a closer look at those changes.
aduth
left a comment
There was a problem hiding this comment.
In closer inspection of the test changes, I think the classes applied are consistent with the new global mock. Part of me wonders if we should have some "listener" element in the mock, vs. null, so it's clearer in the snapshots that an element is expected to be rendered. Maybe it's difficult to have an element which is cross-platform compatible though. Not a huge concern either way.
| } | ||
| return prevState; | ||
| } ); | ||
| }, [] ); |
There was a problem hiding this comment.
Shouldn't this include setMeasurements in the hook dependencies?
There was a problem hiding this comment.
I've tested it and looks like setMeasurements is not necessary since it doesn't change - it's called only once onLayout changed.
There was a problem hiding this comment.
Ah, I didn't know this:
React guarantees that setState function identity is stable and won’t change on re-renders. This is why it’s safe to omit from the useEffect or useCallback dependency list
koke
left a comment
There was a problem hiding this comment.
This is looking pretty good, I left some comments, but this is ready to go on my side once those are addressed
|
I have tested it in Column block PR and it work as expected ! |
Description
Fixes: #19779
This PR introduces new cross-platform hook with an identical API called
useResizeObserverwhich allows listening to the resize event of any target element when it changes sizes.Usage example:
How has this been tested?
You can test it e.g in mobile
SpacerorButtoncomponent. To test editspacer/edit.js/spacer/edit.native.jsin that way:import { useResizeObserver } from '@wordpress/compose';{resizeObserver}useResizeObservervalue and then rotate the device to check if your sizes changed!Screenshots
Types of changes
Introducing new cross-platform hook.
On the mobile platform, it uses invisible
ViewwithonLayoutmethod which generates measurements and reacts on all changes.On the web, it uses
react-resize-awarelibrary under the hood.Checklist: