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

New mobile HOC for comparing container width with breakpoints #19779

Closed
lukewalczak opened this issue Jan 21, 2020 · 6 comments · Fixed by #19918
Closed

New mobile HOC for comparing container width with breakpoints #19779

lukewalczak opened this issue Jan 21, 2020 · 6 comments · Fixed by #19918
Labels
[Type] Enhancement A suggestion for improvement.

Comments

@lukewalczak
Copy link
Member

Is your feature request related to a problem? Please describe.

During the work over Column mobile block turned out that nested Media&Text component is not presented in a stacked layout. The issue was caused by the wrong assumption where we were based on property isMobile from withViewportMatch which was calculated on device width, however, in our case, we should rely on a parent

Describe the solution you'd like

I've prepared the quick solution within a PR, however I think of a more complex solution.

Describe alternatives you've considered

The more complex proposed solution is creating new HOC for that functionality which will get a container width and compare it with breakpoints.

It should work in a similar way to existing HOC withViewportMatch.

@lukewalczak
Copy link
Member Author

My idea of new HOC for measuring container width assumes that container width will be calculated using onLayout method and result will be passed to props as an object with properties corresponding to the mobile breakpoints.

const BREAKPOINTS = {
	medium: 480,
	large: 768,
}

const withContainerMatch = (queries) => createHigherOrderComponent(
	( WrappedComponent ) => ( props ) => {

		const formattedQueries = compact(isArray(queries) ? queries : [queries]);

		const [ currentContainerWidth, setContainerWidth ] = useState( null );
		const [ containerSizes, setContainerSize ] = useState( null );

		const onLayout = ({ nativeEvent }) => {
			const { width } = nativeEvent.layout
			const initialContainerSize = { isSmall: false, isMedium: false, isLarge: false };

			if ( width !== currentContainerWidth ) {
				setContainerWidth(width)
				if ( width < BREAKPOINTS.medium ) {
					setContainerSize({...initialContainerSize, isSmall: true})
				} else if ( width >= BREAKPOINTS.medium && width < BREAKPOINTS.large) {
					setContainerSize({...initialContainerSize, isMedium: true})
				} else if ( width >= BREAKPOINTS.large ) {
					setContainerSize({...initialContainerSize, isLarge: true})
				}
			}
		}

		const finalContainerSizes = {
			...(! isEmpty(formattedQueries) ? { containerSizes }: {}),
			...pick(containerSizes, formattedQueries)
		}

		return (
			<WrappedComponent
				{ ...props }
				onLayout={ onLayout }
				{...finalContainerSizes}
			/>
		)


	},
	'withContainerMatch'
);

Example of usage:

no query passed result
withContainerMatch() this.props.containerSizes: {isSmall: true, isMedium: false, isLarge: false}
one query passed result
withContainerMatch( 'isSmall' ) this.props.isSmall: true
two queries passed result
withContainerMatch( [ 'isSmall', 'isMedium' ] ) this.props.isSmall: true this.props.isMedium: false

@lukewalczak lukewalczak added the [Type] Enhancement A suggestion for improvement. label Jan 22, 2020
@koke
Copy link
Contributor

koke commented Jan 27, 2020

I like the idea of having a HOC or hook to take care of this, but I'm not sure the breakpoints set for @wordpress/viewport are equally useful here, and it might be better to allow callers to pass an arbitrary width.

@lukewalczak
Copy link
Member Author

but I'm not sure the breakpoints set for @wordpress/viewport are equally useful here

I share your opinion, that's why I decided to have only 2 mobile breakpoints such as medium and large which should be enough.

it might be better to allow callers to pass an arbitrary width.

I assumed you've meant passing a width which we want to compare with width from onLayout, e.g: withContainerMatch('480') which results in true/false. However what should be an output in case where callers want to pass array of widths, e.g.: withContainerMatch('200', '400', '600')?.
If you're thinking about totally different approach please let me know.

@lukewalczak
Copy link
Member Author

lukewalczak commented Jan 27, 2020

Also, there is an option to pass a width and compare it similarly to the viewport HOC:

withContainerMatch({ isMedium: '< 480', isLarge: '> 768' }) and the result will be this.props.isMedium: true this.props.isLarge: false

@koke
Copy link
Contributor

koke commented Jan 27, 2020

Also, there is an option to pass a width and compare it similarly to the viewport HOC:

withContainerMatch({ isMedium: '< 480', isLarge: '> 768' }) and the result will be this.props.isMedium: true this.props.isLarge: false

I think I like that API more rather than having predefined breakpoints in the HOC. But I also can't think of more examples other than media-text right now, so I don't have enough use cases in mind to know what the right solution would be. I think I would start with allowing custom queries only, and keep an eye on how it's used and consolidate breakpoints if there are common ones being used.

@lukewalczak
Copy link
Member Author

I think I would start with allowing custom queries only, and keep an eye on how it's used and consolidate breakpoints if there are common ones being used.

Sounds good, thanks! Will start working on that

But I also can't think of more examples other than media-text right now

I was discussing with @jbinda and this kind of HOC will be helpful as well in Columns block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants