-
Notifications
You must be signed in to change notification settings - Fork 56
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/experimental grid block all controls #3444
Conversation
@@ -23,6 +23,9 @@ addAction( 'native.render', 'gutenberg-mobile', ( props ) => { | |||
setupJetpackEditor( | |||
props.jetpackState || { blogId: 1, isJetpackActive: true } | |||
); | |||
if ( __DEV__ ) { | |||
require( './block-experiments-setup' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why this needs to be required dynamically. Could we instead import { registerBlock as registerLayoutGridBlock } from '../block-experiments/blocks/layout-grid/src';
here, and invoke it under __DEV__
? If this is for the incidental idempotency given by require.cache
, maybe we can explicitly make this idempotent by setting a flag in the file-level closure here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that a require
is necessary because metro doesn't support require imports just yet. See #2582 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment refers to why we would use require()
instead of import()
, which are both dynamic. What I'm wondering is, why are we aiming for this to be dynamic? I.e. why not a static import at the top of the file, and dynamically register the block under __DEV__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I think part of the reason why I included it like this is so that we don't load the js script when the flag is not set. but since we are not downloading the js I guess this part doesn't really matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to explore something like this down the road for lazy-loading / code splitting.
@@ -0,0 +1,5 @@ | |||
// This file is to set up the jetpack/layout-grid block that currently lives in block-experiments/blocks/layout-grid | |||
|
|||
import { registerBlock } from '../block-experiments/blocks/layout-grid/src'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a thought about the relative import path used here. Not suggesting a change, but wondering if there's a way to scope this (similar to how we resolve @wordpress
dependencies) such that a future change in project structure wouldn't require any changes here. Something like '@something/blocks'
🤔
If there's too much "magic" in the module resolution configuration, perhaps we'd still achieve a similar effect by importing from an index at a higher level in ../block-experiments
? It feels like "reaching deep into" the path here will be brittle to structure changes in the dependency. (This comment might belong there as well as here 😅 ) Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a great idea but I am not sure how to go about doing that. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second idea would just involve creating an index.js
(or possibly index.native.js
) file in ../block-experiments/blocks/
and re-exporting the block registration there. This would shift the responsibility of maintaining the deeper path traversals to the dependency, which probably makes more sense, since that's where the structure changes would occur.
Regarding the module resolution magic, there is a plugin that might be worth investigating for this purpose, but I haven't tried it out. It essentially allows you to create an alias for the imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the module resolution magic, there is a plugin that might be worth investigating for this purpose, but I haven't tried it out. It essentially allows you to create an alias for the imports.
I've used that plugin in every React native project I've worked on, it's very useful. Also very easy to set up (usually).
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
bbc7b6b
to
9f71767
Compare
Closing this since it was already merged. |
I am closing this since it is not needed any more. |
Adds the submodule Block experiements.
Related PR: Automattic/block-experiments#187
iOS: wordpress-mobile/WordPress-iOS#16423
Android PR:
wordpress-mobile/WordPress-Android#14578
To test:
run
git submodule update
npm run start
PR submission checklist: