Add mechanism to set a width on withViewportMatch #17085

jorgefilipecosta · 2019-08-19T12:39:52Z

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 · 2019-12-09T12:28:50Z

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 · 2019-12-10T17:16:23Z

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

youknowriad · 2019-12-11T07:19:57Z

packages/compose/src/hooks/use-viewport-match/index.js

+ '>=': ( breakpointValue, width ) => ( width >= breakpointValue ),
+};
+
+const ViewportMatchWidthContext = createContext( null );


Maybe just ViewportWidthContext.

youknowriad

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 · 2019-12-11T11:39:36Z

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

aduth · 2019-12-11T14:11:47Z

packages/compose/src/hooks/use-viewport-match/index.js

 };

+useViewportMatch.__experimentalWidthProvider = ViewportMatchWidthContext.Provider;


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

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.

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.

aduth · 2019-12-11T14:13:17Z

packages/compose/src/hooks/use-media-query/index.js

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


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)?

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.

aduth · 2019-12-11T14:13:26Z

packages/compose/src/hooks/use-viewport-match/index.js

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

+const OPERATOR_EVALUATORS = {


Can we document these?

The object was documented.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

aduth · 2019-12-11T17:19:34Z

packages/edit-post/src/editor.js

- </ErrorBoundary>
- <PostLockedModal />
- </EditorProvider>
+ <useViewportMatch.__experimentalWidthProvider value={ undefined }>


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.

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.

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

But it was merged? 🤔

Oh, I see #19075 now. Will review 👍

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

the raised concerns were clarified

jorgefilipecosta requested review from ajitbohra, talldan and youknowriad as code owners August 19, 2019 12:39

jorgefilipecosta added Mobile Web Viewport sizes for mobile and tablet devices [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels Aug 19, 2019

This was referenced Oct 15, 2019

Add simulate media queries #17946

Closed

Use mobile UI in customiser blocks #17960

Closed

jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from 7d7e1cc to c6f260f Compare October 15, 2019 19:08

jorgefilipecosta requested review from adamsilverstein, aduth, atimmer, chrisvanpatten, daniloercoli, dmsnell, ellatrix, etoledom, gziolo, karmatosed, mapk, mkaz, mmtr, nerrad, noisysocks, oandregal, ntwb, SergioEstevao, Soean, swissspidy and TimothyBJacobs as code owners October 15, 2019 19:08

jorgefilipecosta changed the base branch from add/mechanism-to-convert-media-queries to master October 15, 2019 19:13

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Dec 5, 2019

jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from b761220 to 115251d Compare December 9, 2019 12:25

jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from 115251d to ab4ac2c Compare December 10, 2019 17:15

jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Dec 10, 2019

jorgefilipecosta added 2 commits December 10, 2019 19:35

Add mechanism to set a width on useViewportMatch

a60f8fe

Add unit test cases

8cf3bb4

jorgefilipecosta force-pushed the add/mechanism-to-set-a-width-on-withViewportMatch branch from ab4ac2c to 8cf3bb4 Compare December 10, 2019 19:41

Remove the simulation value so end to end tests pass

0e67d94

youknowriad reviewed Dec 11, 2019

View reviewed changes

etoledom removed request for nerrad, adamsilverstein, TimothyBJacobs, ellatrix, dmsnell and etoledom December 11, 2019 07:56

youknowriad approved these changes Dec 11, 2019

View reviewed changes

aduth reviewed Dec 11, 2019

View reviewed changes

jorgefilipecosta and others added 3 commits December 11, 2019 16:17

Update packages/compose/src/hooks/use-media-query/index.js

a199290

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

Document OPERATOR_EVALUATORS object.

7cb403e

Update readme

8e1dbe0

aduth reviewed Dec 11, 2019

View reviewed changes

jorgefilipecosta merged commit 255da17 into master Dec 11, 2019

jorgefilipecosta deleted the add/mechanism-to-set-a-width-on-withViewportMatch branch December 11, 2019 18:37

jorgefilipecosta mentioned this pull request Dec 11, 2019

Revert: File unintentionally merged in PR#17085 #19075

Merged

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mechanism to set a width on withViewportMatch #17085

Add mechanism to set a width on withViewportMatch #17085

jorgefilipecosta commented Aug 19, 2019 •

edited

jorgefilipecosta commented Dec 9, 2019

jorgefilipecosta commented Dec 10, 2019

youknowriad Dec 11, 2019 •

edited

youknowriad left a comment

jorgefilipecosta commented Dec 11, 2019

aduth Dec 11, 2019

jorgefilipecosta Dec 11, 2019

aduth Dec 11, 2019

aduth Dec 11, 2019

jorgefilipecosta Dec 11, 2019

aduth Dec 11, 2019

jorgefilipecosta Dec 11, 2019

aduth Dec 11, 2019

jorgefilipecosta Dec 11, 2019

aduth Dec 11, 2019

aduth Dec 11, 2019

jorgefilipecosta Dec 11, 2019

		};

		useViewportMatch.__experimentalWidthProvider = ViewportMatchWidthContext.Provider;

Add mechanism to set a width on withViewportMatch #17085

Add mechanism to set a width on withViewportMatch #17085

Conversation

jorgefilipecosta commented Aug 19, 2019 • edited

Description

How has this been tested?

jorgefilipecosta commented Dec 9, 2019

jorgefilipecosta commented Dec 10, 2019

youknowriad Dec 11, 2019 • edited

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Aug 19, 2019 •

edited

youknowriad Dec 11, 2019 •

edited