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

Add mechanism to set a width on withViewportMatch #17085

Merged

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Aug 19, 2019

Description

This PR adds a mechanism to make withViewportMatch usage evaluate as if the viewport had a given width.
This will allow us to dynamically change the with of the editor to simulate mobile usages and allows us now to make the editor appear in the customizer as it appears on mobile.

We create a new component ViewportMatchWidthProvider that accepts width as a property, and when used all with/ifViewportMatch descendants of it evaluate as if the viewPort had the specified width.

How has this been tested?

I tested on the branch from this PR #17960 (which uses this commit) and verified that I get a mobile-like UI in the customizer.

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from b761220 to 115251d Dec 9, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Dec 9, 2019

Hi @aduth, @youknowriad,
This PR was updated to make the simulation happen on useViewportMatch (happening by consequence on withViewPort match because the HOC uses the hook).
The changes in https://github.com/WordPress/gutenberg/blob/add/mechanism-to-set-a-width-on-withViewportMatch/packages/edit-post/src/editor.js are for testing purposes and will not be merged.
To test I changed values on this file and verified the flag passed to the image block was the expected one.

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from 115251d to ab4ac2c Dec 10, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Dec 10, 2019

This PR was updated and unit tests were added, should be ready for a review.

@jorgefilipecosta jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from ab4ac2c to 8cf3bb4 Dec 10, 2019
'>=': ( breakpointValue, width ) => ( width >= breakpointValue ),
};

const ViewportMatchWidthContext = createContext( null );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 11, 2019

Contributor

Maybe just ViewportWidthContext.

Copy link
Contributor

youknowriad left a comment

This looks good but I was wondering, this only affects the viewportMatch behavior (JS checks) and I believe we have another mechanism to do the same for CSS right?

could we combine those somehow? Meaning if we add the provider to a React tree, the provider also triggers the CSS forcing inside that tree?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Dec 11, 2019

This looks good but I was wondering, this only affects the viewportMatch behavior (JS checks) and I believe we have another mechanism to do the same for CSS right?

could we combine those somehow? Meaning if we add the provider to a React tree, the provider also triggers the CSS forcing inside that tree?

Hi @youknowriad, I guess the mechanisms make sense used together but I separated in two PR's to make reviewing easier. My plan is to provide a single component that uses the mechanism to simulate media queries and the mechanism to simulate the value on the hook (uses both components to combine the mechanisms).

packages/compose/src/hooks/use-media-query/index.js Outdated Show resolved Hide resolved
};

useViewportMatch.__experimentalWidthProvider = ViewportMatchWidthContext.Provider;

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Is there any prior art for how we assign context as properties of hooks? Wondering if it should be its own exported member instead.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 11, 2019

Author Member

No, I think we never did this before. I exported as a property of the hook to make clear that the component only affects the hook return value. But I can export an __experimentalUseViewportMatchWidthProvider component instead. I don't have a preference here. If you have a preference for the component version, let me know, and I will perform the update.

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Hm. The main thing that caught my eye was in how it's used:

<useViewportMatch.__experimentalWidthProvider value={ undefined } />

Technically it's valid, but a little non-conventional.

For me, if it was a separate thing, I don't think we would want to associate it so strongly to being only relevant for this hook, so I would imagine something like ViewportWidthProvider being sufficient.

But especially given that this package is meant to be used exclusively for these sorts of helpers (higher-order components and hooks), I think it would be fair to associate it directly with the hook.

For that reason, I think what you've proposed here can be okay.

* @return {boolean} return value of the media query.
*/
export default function useMediaQuery( query ) {
const [ match, setMatch ] = useState( false );
useEffect( () => {
if ( ! query ) {

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

I get that hooks need to be rendered deterministically, but it still feels odd to me that we'd want to support this use case of an empty query for useMediaQuery. Do the default hooks support this practice? i.e. useCallback( undefined ) etc? Are there patterns in the hooks landscape for this sort of "forking" logic (i.e. do different behavior based on some condition)?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 11, 2019

Author Member

Hi @aduth, I verified that useCallback( undefined ) returns undefined useCallback( false ) returns false, this behavior is equivalent to what we did useMediaQuery.
Without this pattern, I am not aware of any way we could implement this hook and avoid calling the media query listeners even when they are not needed because the value is simulated.

@@ -37,6 +42,13 @@ const CONDITIONS = {
'<': 'max-width',
};

const OPERATOR_EVALUATORS = {

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Can we document these?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 11, 2019

Author Member

The object was documented.

jorgefilipecosta and others added 3 commits Dec 11, 2019
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
</ErrorBoundary>
<PostLockedModal />
</EditorProvider>
<useViewportMatch.__experimentalWidthProvider value={ undefined }>

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Do we need to force that this provider be rendered in the application? Isn't it enough for useContext to use a default when there's no explicit context value?

It would make the hook easier to use in other applications. And, unless I'm mistaken, it should "just work" if this wrapper element were removed here.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 11, 2019

Author Member

Hi @aduth, this file is not supposed to be merged. I just added this here to make it easier to test, one can just add value here and see if the value is applied and the editor reacts differently.

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Hi @aduth, this file is not supposed to be merged.

But it was merged? 🤔

This comment has been minimized.

Copy link
@aduth

aduth Dec 11, 2019

Member

Oh, I see #19075 now. Will review 👍

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 11, 2019

Author Member

Yes, I'm sorry for that I prepared the revert and missed the push :(

@jorgefilipecosta jorgefilipecosta dismissed mapk’s stale review Dec 11, 2019

the raised concerns were clarified

@jorgefilipecosta jorgefilipecosta merged commit 255da17 into master Dec 11, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the add/mechanism-to-set-a-width-on-withViewportMatch branch Dec 11, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.