Skip to content
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

[RNMobile] Create cross platform useResizeObserver() hook #19918

Merged
merged 12 commits into from
Mar 13, 2020

Conversation

lukewalczak
Copy link
Member

@lukewalczak lukewalczak commented Jan 27, 2020

Description

Fixes: #19779

This PR introduces new cross-platform hook with an identical API called useResizeObserver which allows listening to the resize event of any target element when it changes sizes.

Usage example:

  • web
const App = () => {
	const [ resizeListener, sizes ] = useResizeObserver();
	return (
		<div>
			{ resizeListener }
			Your content here
		</div>
	);
};
  • mobile
const App = () => {
	const [ resizeListener, sizes ] = useResizeObserver();
	return (
		<View>
			{ resizeListener }
			Your content here
		</View>
	);
};

How has this been tested?

You can test it e.g in mobile Spacer or Button component. To test edit spacer/edit.js / spacer/edit.native.js in that way:

  1. import hook

import { useResizeObserver } from '@wordpress/compose';

  1. use a hook - fill parent's boundaries with {resizeObserver}
spacer/edit.native.js


const [ resizeListener, sizes ] = useResizeObserver();

...

<View>
	{resizeListener}
	<View
		style={ [
			defaultStyle,
			isSelected && styles.selectedSpacer,
			{ height },
		] }
	>
		<InspectorControls>
			<PanelBody title={ __( 'Spacer settings' ) }>
				<RangeControl
					label={ __( 'Height in pixels' ) }
					minimumValue={ minSpacerHeight }
					maximumValue={ sliderSpacerMaxHeight }
					separatorType={ 'none' }
					value={ height }
					onChange={ changeAttribute }
					style={ styles.rangeCellContainer }
				/>
			</PanelBody>
		</InspectorControls>
	</View>
</View>


  1. log / use the useResizeObserver value 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 View with onLayout method which generates measurements and reacts on all changes.
On the web, it uses react-resize-aware library under the hood.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@aduth
Copy link
Member

aduth commented Jan 27, 2020

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.

@lukewalczak
Copy link
Member Author

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?

@aduth
Copy link
Member

aduth commented Jan 28, 2020

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 (withSelect, for example), but to me those only continue to exist purely for backward-compatibility for a time when hooks did not exist. I think if we were in a position to add them now, we would bypass the higher-order component altogether and implement only the hook.

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)

@youknowriad
Copy link
Contributor

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.

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.

@lukewalczak
Copy link
Member Author

Basically, functionality of that HOC is really similar to useViewportMatch, however it's based on native onLayout calculation and it's dedicated only for mobile platform. It will be used generally in mobile Columns block which is being created or in existing Media&Text which is a class component, that's why I asked wdyt about creating HOC layer after rewriting current solution into hook.

@lukewalczak
Copy link
Member Author

I rewrote a functionality to hook called useContainerMatches. Let me know wdyt ✌️

@lukewalczak lukewalczak changed the title Create withContainerMatches HOC Create useContainerMatch hook Jan 29, 2020
@lukewalczak lukewalczak changed the title Create useContainerMatch hook [RNMobile] Create useContainerMatch hook Jan 29, 2020
@lukewalczak lukewalczak marked this pull request as ready for review January 29, 2020 15:53
@youknowriad
Copy link
Contributor

I was wondering if there's a way to have the exact same API as the web version useViewportMatch with a custom native implementation?

@lukewalczak
Copy link
Member Author

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.

@aduth
Copy link
Member

aduth commented Jan 30, 2020

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 useViewportMatch to support arbitrary widths. Or the native implementation could override the defaults to provide its own named breakpoint values which make more sense in a mobile context.

@lukewalczak
Copy link
Member Author

lukewalczak commented Jan 30, 2020

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 Columns we can manually set the the percentage width, so then I have to know what are the current measurements to properly stack elements.
Please let me know if the answer is more clear right now.

@aduth
Copy link
Member

aduth commented Jan 30, 2020

Ah! Is the requirement more like what was explored in #18745 where react-resize-aware was used to measure width changes within a particular rendered element, based on ideas similar to the "Element Queries" project?

I could see how that's serving a different purpose than just viewport dimensions. And I suppose react-resize-aware would not be usable here, since I imagine it relies on a DOM? I still might wonder, especially if we have need for this sort of behavior in the browser as well, whether we could devise a single hook that can work well in either environment. The challenge I could see is that both have some specific requirement out of the element it's measuring: In the native implementation, the View's onLayout, and in react-resize-aware, the resizeListener measuring node. Maybe it's okay for this to be distinct in each environment, as long as the interface of the hook is otherwise reasonably consistent, and it is documented clearly.

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?

@lukewalczak
Copy link
Member Author

lukewalczak commented Jan 30, 2020

Your comparison example presented it correctly. Exactly, my mobile hook is really similar to react-resize-aware, but as you noticed the library depends on DOM manipulation, so it won't be useful.

@pinarol pinarol requested a review from koke February 4, 2020 10:13
@lukewalczak lukewalczak self-assigned this Feb 4, 2020
@lukewalczak lukewalczak added the [Package] Hooks /packages/hooks label Feb 4, 2020
@lukewalczak lukewalczak force-pushed the rnmobile/with-container-matches-hoc branch from 0a1691f to 64f894a Compare February 5, 2020 09:37
@koke
Copy link
Contributor

koke commented Feb 5, 2020

Do these ideas make sense? Or are these requirements not as aligned as I'm thinking them to be?

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 use-resize-observer hook library that has polyfills for the API.

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 onLayout prop.

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 react-resize-aware and make a light wrapper around it, or switch to use-resize-observer eventually.

For native, we can use a similar approach to what react-resize-aware does: their "listener" is like having an invisible View set to fill the parent's boundaries (i.e. StyleSheet.absoluteFill) with an onLayout listener attached to it.

@koke
Copy link
Contributor

koke commented Feb 5, 2020

I forgot to mention, because this PR was actually implementing useContainerMatch and not just the hook to get dimensions, that I think those two things should be decoupled. We can have the useResizeObserver hook, and then decide if we want another hook for the query matching that uses that, or we can start by leaving that logic for each component until we have a few more use cases.

@lukewalczak lukewalczak requested a review from Soean as a code owner March 2, 2020 11:25
@lukewalczak lukewalczak changed the title [RNMobile] Create useContainerMatch hook [RNMobile] Create cross platform useResizeObserver() hook Mar 2, 2020
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I like how this is coming together 👍

packages/compose/README.md Outdated Show resolved Hide resolved
@lukewalczak
Copy link
Member Author

@aduth Thanks for the valuable feedback! I hope that correctly resolved your suggestions.

Comment on lines -5 to -14
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"
/>
Copy link
Member

Choose a reason for hiding this comment

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

These snapshot changes do not seem like they are sensible, since they undo the fixes intended with #19825.

Copy link
Member

Choose a reason for hiding this comment

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

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 aduth added the [Type] New API New API to be used by plugin developers or package users. label Mar 2, 2020
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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.

@pinarol pinarol mentioned this pull request Mar 9, 2020
6 tasks
packages/compose/README.md Outdated Show resolved Hide resolved
}
return prevState;
} );
}, [] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this include setMeasurements in the hook dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested it and looks like setMeasurements is not necessary since it doesn't change - it's called only once onLayout changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

https://reactjs.org/docs/hooks-reference.html#usestate

Copy link
Contributor

@koke koke left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, I left some comments, but this is ready to go on my side once those are addressed

@jbinda
Copy link
Contributor

jbinda commented Mar 10, 2020

I have tested it in Column block PR and it work as expected !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Hooks /packages/hooks [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New mobile HOC for comparing container width with breakpoints
7 participants