[RNMobile] Added layoutGridBlock enabling capabilities flag to mobile gutenberg bridge #31777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
enejb
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.62 MB ℹ️ View Unchanged
|
enejb
force-pushed
the
mobile/add/layout-grid-capabilities-check
branch
from
9d3fc14 to
d88d987
Compare
This was referenced May 17, 2021
Merged
… gutenberg bridge
enejb
force-pushed
the
mobile/add/layout-grid-capabilities-check
branch
from
d88d987 to
eefd6c7
Compare
4 tasks
chipsnyder
reviewed
May 21, 2021
Comment on lines
19
to
21
| case contactInfoBlock | ||
| case layoutGridBlock | ||
| case mediaFilesCollectionBlock |
Contributor
There was a problem hiding this comment.
@mchowning @hypest I'd be curious to get your thoughts on these three capabilities (contactInfoBlock, mediaFilesCollectionBlock, and layoutGridBlock). This becomes a not so secretive passthrough for jetpack content. I wonder if we should make this more universal by changing this capability to something like additionalSupportedBlocks that contains an array of the block names the client wants to support.
audioBlock might be a good candidate to include in this as well. WDYT?
To be clear @enejb is following the established pattern here and incorporating my change suggestion would increase the scope of this PR so I'm happy to take that on under a different PR. I wanted to see if there was any existing context or if it's just the evolution of the architecture.
Contributor
There was a problem hiding this comment.
I wonder if we should make this more universal by changing this capability to something like additionalSupportedBlocks that contains an array of the block names the client wants to support.
I agree. I think it's fine for this PR, but this is something we should fix.
Contributor
There was a problem hiding this comment.
Agreed as well. I think it's easier to see this when you have enough occurrences that it starts to become a pattern. That's the right time to make an abstraction with the characteristics that are common to all 👍
Contributor
There was a problem hiding this comment.
+1 to what the fine folks above be already said, for generalizing/abstracting the "block offerability" flags.
Contributor
Author
|
@chipsnyder Are there any changes required on this PR? |
Contributor
|
I think we're agreed that the approach looks fine for this PR. I'll spin up a separate task to handle the changes that were discussed. I see this is still in draft, so I'm unsure if you have more testing or planned changes. |
hypest
reviewed
Jun 3, 2021
Contributor
hypest
left a comment
hypest
left a comment
There was a problem hiding this comment.
Took a look at the Android side changes and look good to me. That said, I see that @antonis was probably reviewing the WPAndroid PR so, I think it makes sense to check this one out in combination. @antonis wdyt?
Member
antonis
approved these changes
Jun 3, 2021
Member
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
Great work @enejb 👍
The code additions LGTM and follow the established pattern 🎉
As noted we may need to change this but this would be a separate PR.
chipsnyder
approved these changes
Jun 3, 2021
Contributor
chipsnyder
left a comment
chipsnyder
left a comment
There was a problem hiding this comment.
This piece looks good from my side as well. But Echoing what Antonis mentioned this shouldn't be merged until wordpress-mobile/gutenberg-mobile#3513 is ready to go as well.
7 tasks
Contributor
|
Is this PR still something we want to merge, or should we close it? |
Member
@mchowning I'll close this since the changes on this PR are already merged with #32495. @enejb Please reopen if there is something more on this 🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update the mobile bridge to add the layoutGridBlock capability flag.
Similarly to #28293
Related PR wordpress-mobile/gutenberg-mobile#3513.
Types of changes
Adding a new capability flag that lets the mobile app Gutenberg editor know when the layout grid block is present on the site.
Checklist:
*.native.jsfiles for terms that need renaming or removal).