[RNMobile] Enable reusable block only in WP.com sites #31744
Conversation
fluiddot
added
Mobile App - i.e. Android or iOS
|
Size Change: +3 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
| this.getEditorSettings = memize( | ||
| ( settings, capabilities ) => ( { | ||
| ...settings, | ||
| capabilities, | ||
| } ), | ||
| { | ||
| maxSize: 1, | ||
| } | ||
| ); |
There was a problem hiding this comment.
I added the capabilities into the editor settings that are passed to the EditorProvider, this is required because we need them when executing the useNativeBlockEditorSettings hook.
Additionally, to prevent extra re-renders, I decided to memoize the editor settings object.
There was a problem hiding this comment.
@fluiddot I was looking around the codebase for some context about what maxSize does but I wasn't able to figure it out. Could you please expound a bit on its usage?
There was a problem hiding this comment.
Is it utilized to define the maximum size of the cache?
There was a problem hiding this comment.
Exactly, this parameter controls the size of the cache, here you can check the documentation of the memize package.
There was a problem hiding this comment.
There’s a similar logic in this file:
There was a problem hiding this comment.
Good good. Thanks for confirming 🙇🏾
| reusableBlocks: isWeb | ||
| ? select( coreStore ).getEntityRecords( | ||
| 'postType', | ||
| 'wp_block', | ||
| { per_page: -1 } | ||
| ) | ||
| : [], // Reusable blocks are fetched in the native version of this hook. |
There was a problem hiding this comment.
Reusable blocks are fetched in the native version of this hook, so we only fetch them for the web version.
| */ | ||
| import useBlockEditorSettings from './use-block-editor-settings.js'; | ||
|
|
||
| function useNativeBlockEditorSettings( settings, hasTemplate ) { |
There was a problem hiding this comment.
The purpose of this hook is to add custom code to the useBlockEditorSettings hook, which will be only executed in the native version of the editor.
There was a problem hiding this comment.
Thanks for the context here. I can see that useNativeBlockEditorSettings is utilizing the web variant to create the editorSettings and then based on if support for reusable block is enabled or not based on the site type, we are then querying the coreStore for the said blocks.
There was a problem hiding this comment.
Yeah exactly, the main difference between this version and the web is that capabilities is only available in native. This is used, as you commented, to control whether it should query the reusable blocks.
|
I've realized that the reusable block is disabled on Atomic sites, I'll push a fix as soon as possible into the WPiOS and WPAndroid branches 💨 . |
Appreciated! 👍🏾 |
I've just committed the fix for the Atomic sites to the main app branches, additionally, I've added an extra test case for Atomic sites because I wasn't covering it initially. |
|
Looks like the case of an empty reusable block is not properly handled, here's a screenshot of how it looks like currently:
Potential solutionsI was thinking on different options for this: Empty content
Warning box
Warning box as the content
To be honest, it's kind of an edge case to have empty reusable blocks although it's possible to create them. In any case, I don't see the best option for this so I'd appreciate your feedback. cc @iamthomasbishop Thanks for your help 🙇 ! |
# Conflicts: # packages/react-native-editor/CHANGELOG.md
# Conflicts: # packages/react-native-bridge/android/react-native-bridge/src/main/java/org/wordpress/mobile/WPAndroidGlue/GutenbergProps.kt # packages/react-native-bridge/ios/GutenbergBridgeDelegate.swift # packages/react-native-editor/android/app/src/main/java/com/gutenberg/MainActivity.java
# Conflicts: # packages/editor/src/components/provider/use-block-editor-settings.js # packages/react-native-editor/CHANGELOG.md
There was a problem hiding this comment.
Since the issue raised is not blocking this PR and affects all unsupported blocks, we can go ahead with the approval here. LGTM 🚢 The unselected empty state issue that was raised in the comments for design feedback can be implemented in another PR if you think that's best 👍🏾
|
@fluiddot I believe what you have implemented is the expected behavior. I took a look at the web editor and can confirm this is the case (screenshot). I could see a case to be made for somehow making the unselected state "invisible", but for now I think what you have here is fine. In a future iteration, it might be worth customizing the placeholder text to say |
Thanks @iamthomasbishop for the feedback ❤️ ! Initially, I also thought to use the implementation we have in the web editor but in this case, since the reusable block is not editable in the app yet, I think it can be a bit misleading, the user most likely would try to tap on it but it will be disabled. In any case, I'll open an issue regarding this because the empty paragraph gets focused when the block is inserted and it's even more confusing (check the attached videos) 😄 .
|
@fluiddot Oh duh, of course 🤦 In that case, I think the best solution would be to at least show a tap target with the dotted outline when the block isn't selected, even if there isn't anything within it. We could theoretically show a placeholder of something like Does that make sense? I'm open to other suggestions. |
One of the options I suggested was to add a placeholder, here is an example:
It's not ideal but it could work until we support editing, @iamthomasbishop wdyt? |
fluiddot
force-pushed
the
rnmobile/add/reusable-block-capability
branch
from
228e7ac to
233e227
Compare
|
I've just created this issue as a follow-up for the reusable block with empty content issue. |
@fluiddot Sorry, I don't think I was very clear in my suggestion. What I was suggesting is a change to the placeholder text label (
However, I do think a mix between your placeholder approach and my idea of adding an outline (especially on the non-selected state) would probably work well. Something like this:
One other minor suggestion I'd like to make is that we might want to decrease the height of the placeholder bit you have (with the light gray background). I'm guessing that we're using the existing placeholder component in your example above, but if it's not a hassle would it be possible to slim down the vertical padding a little bit? I believe the top/bottom padding on the container is 24px, but we can bring it down to 12px.
Altogether, here is what that would look like between the selected and non-selected states:
Any thoughts? I think the only big change here is the outline on the non-selected state. The compact spacing proposed is less important, but would be nice. |
# Conflicts: # packages/react-native-editor/CHANGELOG.md
|
@jd-alexander Heads up that I've made a small change in the PR to disable reusable blocks in the demo project in this commit. It doesn't affect the feature itself so I think it's not really necessary an extra review. |
…-take-2 * trunk: (57 commits) Image block: fix cover transform and excessive re-rendering (#32102) compose: Add types to useMergeRefs (#31939) Navigation: Fix collapsing regression. (#32081) components: Promote Elevation (#31614) compose: Add types to useReducedMotion and useMediaQuery (#31941) Update the graphic that appears in the Template Editor welcome guide (#32055) Block Navigation: use CSS for indentation with known max indent instead of spacer divs (#32063) Fix broken template part converter modal styles. (#32097) compose: Add types to `usePrevious` (#31944) components: Add ZStack (#31613) components: Fix Shortcut polymorphism (#31555) compose: Add types to `useFocusReturn` (#31949) compose: Add types to `useDebounce` (#32015) List View: Simplify the BlockNavigation component (#31290) Remove query context leftovers (#32093) Remove filter_var from blocks (#32046) Templates: Remove now-obsolete gutenberg_get_template_paths() (#32066) [RNMobile] Enable reusable block only in WP.com sites (#31744) Rename ViewOwnProps to PolymorphicComponentProps (#32053) Rich text: remove inline display warning (#32013) ...
Now it's super clear, thank you very much @iamthomasbishop for the clarification! I agree, it makes sense to display it this way, I'll take your insights and update the issue. Regarding the height decrease of the placeholder:
What do you think about applying the decrease in the component itself? It will affect all places where we're using it but I think it makes sense. |
Sounds good, thank you!
I have a feeling it might be a bit too compact for some of the other instances, but I'm definitely open to trying it to see how it looks/feels! And I wouldn't want this to block or slow progress on the rest of this work, so feel free to break that off into a separate ticket if preferred. 😃 |
Ok, I'll check how it looks like on other places where we're using it just in case. For this specific case, probably what I'll do is to apply the fix only for the reusable block. |
gutenberg-mobilePR: wordpress-mobile/gutenberg-mobile#3490WordPress-iOSPR: wordpress-mobile/WordPress-iOS#16475WordPress-AndroidPR: wordpress-mobile/WordPress-Android#14623Description
This PR adds a capability into the editor to limit the availability of the reusable block to WP.com sites, until we manage to address the issue related to fetching reusable blocks in self-hosted sites.
When the new capability
reusableBlockisfalse, we do the following actions:core/block(reusable block), this way we prevent the reusable block data to be fetched in case it's already in the post (code reference)How has this been tested?
We have to test that the reusable block is enabled in WP.com sites but not on self-hosted:
WordPress.com site - Reusable block is enabled
Create a WordPress.com site or use an already created one.
WordPress.com site (Atomic) - Reusable block is enabled
Create an Atomic site via WordPress.com site or use an already created one.
Self-hosted site with Jetpack - Reusable block is disabled
Create a self-hosted site with Jetpack (I used https://jurassic.ninja/create/) or use an already created one.
Self-hosted site without Jetpack - Reusable block is disabled
Create a self-hosted site without Jetpack (I created a local one using this instructions) or use your own.
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).