[RNMobile] Introduce Context Constants to Native #35510
Conversation
With this commit, the 'packages/blocks/src/api/registration.js' file is duplicated into a 'native' version. This is necessary as we'll be making native-specific changes to the metadata that's registered alongside a block.
SiobhyB
added
[Feature] Nested / Inner Blocks
|
Size Change: +59 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
'providesNativeInnerBlockContext' and 'usesNativeInnerBlockContext' are intended to replace the default 'providesContext' and 'usesContext'. These will be used to indicate times where a context is only used with the apps and constant (usually in use with inner blocks).
Following this commit, native blocks will now be able to use 'providesNativeInnerBlockContext' in order to provide a context to an inner block.
Following this commit, native blocks will now be able to use 'usesNativeInnerBlockContext' in order to use a context provided by a parent block.
As the 'imageCrop' context is only used with the app, it's flagged with 'providesNativeInnerBlockContext' as part of this commit.
Following 65c3dd6, the 'imageCrop' context was flagged to indicate it's only used with the app. This commit builds on that work to reference it in the image block via 'usesNativeInnerBlockContext'.
We can only look up a block's context if 'blockType' is true. This commit therefore adds a check to ensure 'blockType' is true before creating a context object.
As with 5a7d414, we can only look up a block's context if 'blockType' is true. This commit addresses this issue in a second file.
SiobhyB
changed the title
|
👋 @geriux, I've requested your review as this includes changes to the Gallery block's contexts, which I know you worked on and might have a sense of whether this approach seems sound. Let me know if there's anyone else who you think should review, too. @glendaviesnz, I also wanted to get your thoughts around whether this seems similar to what you had in mind when suggesting context constants. As well as gathering your general thoughts on the code changes here, I'm keen to hear whether the naming of
Thanks in advance! And please do let me know if anything needs clarifying around these proposed changes. 🙇♀️ |
|
Thanks for the ping. A couple of thoughts after taking a look at this: 1/ I wonder if we need the native specific This would require a slightly different approach to context naming, which I haven't seen anywhere but may already be already be seen as a valid approach, and that is to use a different name for the context (name on left below) to the attribute (name on right), eg. To me this makes it easier to create names that are more meaningful when passed down than trying to come up with a single name that is accurate/meaningful in the parent and the children. 2/ The above naming discussion is separate to the idea of context constants. That discussion related to context values that were not being serialized to the block attributes, and The Sorry, that is probably a little long winded. I may be overlooking some native specific issues as I haven't been involved directly in any of the native dev, but it seems like looking at how we name the existing context values rather than creating a new namespace may be an alternative way forward. The addition of a contextConstants namespace for the likes of |
|
@glendaviesnz, I really appreciate your thoughtfulness here, not long-winded at all! It seems there's a good case for keeping
The above contexts would be set to To move forward, and with your feedback in mind, do the following seem reasonable as next steps for me to take?
|
Over to you if you want to worry about a new PR for the name change at this point, was using that as an example to highlight issues with with some of the current naming rather than indicating that it was a critical change we should make. I imagine it will take some wider discussion if we do go that way, as I think there might be some that would prefer to keep the attribute and context names the same for consistency ... but if you are open to starting a conversation on that then an example PR is probably a good place to start.
Yeh, sounds good, will be interesting to see how this looks/works with a real example. |
|
So I'm not as familiar with contexts as @glendaviesnz is, but one thought that I personally had was that there was a lot of repeat of which could be extracted into a function that's used throughout these files. |
Following the discussion here, a new 'fixedHeight' context is added to the Gallery block with this commit: #35510 (comment) This context will be provided by the core Gallery block and possibly other parent blocks in the future. Its default will always be true. It's currently used to set a fixed height on the native Gallery block.
The new context types are renmaed from "*NativeInnerBlockContext" to "*ContextConstants" following feedback here: #35510 (comment)
Discussion here: #35510 (comment)
It should be possible for all platforms to make use of context constants. As such, this commit removes the native-specific registration file, so the PR's changes are now visible in the main registration file and apply to all platforms.
| }, | ||
| "fixedHeight": { | ||
| "type": "boolean", | ||
| "default": true |
There was a problem hiding this comment.
I think the idea with the constants was that they wouldn't be listed as block attributes because of the fact that they would be values that don't change and so aren't serialised in the block content. So if this is the case with fixed height this attribute wouldn't be here and below the constant would be defined as:
"providesContextConstants": {
"fixedHeight": true
},
@talldan is this what you were imagining when you mentioned this idea here?
There was a problem hiding this comment.
@glendaviesnz, thank you for this, what you described makes perfect sense and I've refactored the code accordingly. It'd be great to hear if you (and @talldan) think this is on the right path or if there's anything else that needs to be changed here. 🙇♀️
There was a problem hiding this comment.
(Disclaimer: I'm aware of the error with the unit tests, which is directly related to the introduction of Edit: I've updated the tests to resolve the error in 8719c9e.providesContextConstants. I plan to look into it after first gathering thoughts on the existing approach.)
As per the discussion here, part of the purpose of using context constant is to negate the need for serialisation, as the values won't change: #35510 (comment) As such, with this commit, the function that was evaluating the value of the context constants based on a block's attributes is refactored. With this change, 'providesContextConstants' are added to the block's context object in its entirety, with no need to grab anything from the block's attributes.
SiobhyB
changed the title
|
@SiobhyB - I am busy with some 5.9 release things, but should hopefully have time to look at this before the end of the week. |
|
@glendaviesnz, just checking in, will you have time to look over this this week? |
Thanks for the reminder @SiobhyB. I had a quick look and for some reason on web the Also, I wonder if we want a separate @talldan when you suggested this idea of contextConstants where you thinking that they would be defined separately at parent and child level or just merged in with the other usesContext values at the child level? |
|
Thanks for this @glendaviesnz!
I confess some ignorance on the web side of things, and had focused my testing efforts on mobile, so thank you for bringing this up! I'm seeing the same as you on the web, and haven't been able to figure out why just yet. I'll keep looking into it and report back any findings.
Do you have a preference here? I can see benefits to both approaches and don't have a strong preference myself. Interested to hear @talldan's thoughts too. |
I don't have a strong preference, let's wait and see what @talldan has to say. I will hopefully have more time next week to take another look at why it isn't working on web side. |
|
From the mobile-side, we were able to use block supports to achieve what we needed to in #36618 and #36371. I'm going to go ahead to close this PR as I won't be working on it going forward and, following the discussion at #36582, it's unclear whether there's any other current need for context constants. |
Gutenberg Mobile:wordpress-mobile/gutenberg-mobile#4099Description
The Gallery block was recently refactored, for both the web and the mobile (native) apps, in order to pull in image blocks via the InnerBlocks API.
As part of this refactor for the mobile apps, the following native-specific block contexts were introduced in order to style the gallery's inner image blocks:
imageCrop, was introduced to support the cropping of images.fixedHeight, was introduced in [RNMobile] Refactor Fixed Height Styling for Gallery Block #36013 to explicitly set a fixed height for the images.Similarly, as described in #33531, an
allowResizecontext was introduced on the web-side to disable the image block's resize option for galleries.Problem: Additional contexts for customizing child blocks are going to be necessary.
On the native side, for example, the settings icon is included within the block, unlike on the web, where the block's settings are separated out into the sidebar. The ability to hide the settings icon is therefore likely to be a fairly common use case for parent blocks. There are also likely to be other valid use cases for customizing inner blocks in the future.
Adding additional contexts isn't an ideal or scaleable solution. This has also been discussed in #33531, with some suggestions outlined there.
Proposal: With this PR, the idea of introducing context constants, as initially suggested here, is built upon.
usesContextConstantsis provided as an alternative tousesContextandprovidesContextConstantsas an alternative toprovidesContext.The aim is to explicitly indicate the purpose of these contexts and that they aren't serialised, as would normally be the case with contexts. It is also hoped that the introduction of context constants will make it easier to add these types of contexts in the future.
Note: This PR is native-specific but a separate web-specific PR can be made, building on the work done here, if approved.
How has this been tested?
Prequisite: Enable the flag for the refactored Gallery block. on mobile. At the time of writing, the way to enable the block on the Gutenberg Mobile demo app is to comment out this section of code.
With the refactored Gallery block enabled, we should ensure that the changes introduced with this PR don't have any adverse effects:
fixedHeightcontext constant is working and hasn't caused any regressions (no images would be visible if the context wasn't working).imageCropcontext is still working as expected.Screenshots
The following screencast shows the images going from cropped to uncropped, as expected, to show the
imageCropcontext is working as intended following the changes in this PR:Screen.Recording.2021-10-11.at.17.57.15.mov
Types of changes
This PR includes changes to code and the overall approach that's taken to adding contexts when needed to change inner blocks on the native-side, with the following high-level overview:
providesContextConstantsandusesContextConstantsare added to block metadata upon registration via theblocks/src/api/registration.jsfile.providesContextConstantswas added to the object generated by thegetBlockContextfunction. There are also now checks in place here and here to account for the existence of context constants on the native side.fixedHeight(an existing context, first implemented in [RNMobile] Refactor Fixed Height Styling for Gallery Block #36013) as an example of a context constant.Checklist:
*.native.jsfiles for terms that need renaming or removal).