[RNMobile][Embed block] Block settings #33654
Conversation
ceyhun
added
Mobile App - i.e. Android or iOS
| @@ -6,7 +6,7 @@ import apiFetch from '@wordpress/api-fetch'; | |||
|
|
|||
| // Please add only wp.org API paths here! | |||
| const SUPPORTED_ENDPOINTS = [ | |||
| /wp\/v2\/(media|categories|blocks)\/?\d*?.*/i, | |||
| /wp\/v2\/(media|categories|blocks|themes)\/?\d*?.*/i, | |||
There was a problem hiding this comment.
getThemeSupports selector may trigger a request to /wp/v2/themes?status=active
There was a problem hiding this comment.
I'm wondering if we should fetch theme data via the bridge instead of directly making the API request, for other values like colors or gradients, we're getting them from the editor settings:
gutenberg/packages/react-native-bridge/ios/Gutenberg.swift
Lines 193 to 214 in 59530f3
From my POV it's totally ok to fetch from the editor but we might have issues for supporting offline mode, wdyt?
There was a problem hiding this comment.
I tried fetching it from the bridge on WPiOS via WordPressKit-iOS, but it seems like the /wp-block-editor/v1/settings endpoint is used instead of /wp/v2/themes if the blog supports it here and the /settings endpoint currently doesn't seem to return the responsive-embeds value we need.
There was a problem hiding this comment.
I tried fetching it from the bridge on WPiOS via WordPressKit-iOS, but it seems like the /wp-block-editor/v1/settings endpoint is used instead of /wp/v2/themes if the blog supports it here and the
/settingsendpoint currently doesn't seem to return theresponsive-embedsvalue we need.
Right, I checked the schema of /wp-block-editor/v1/settings (reference) and there's no reference to responsive-embeds, so looks like the theme support settings are only included in the theme model. Now I'm wondering if we should continue fetching the theme via the api-fetch or from the theme service (WordPress-iOS reference and WordPressKit-iOS reference) and expose it in the RN bridge, wdyt?
NOTE: I noticed that the Theme model (reference) doesn't include the theme support settings so, in case we go that way, we should include it.
There was a problem hiding this comment.
I think the response from /sites/%@/themes/mine also doesn't return the responsive-embeds value. I couldn't find any other endpoint that returns it, but could be that I missed out something.
Otherwise I guess we can still fetch it from /wp/v2/themes via BlockEditorSettingsServiceRemote in WordPressKit-iOS with the function here, even when the blog supports /wp-block-editor/v1/settings endpoint. But I think WPiOS is expecting both endpoints to have the same response structure as they are both handed the same completion handler here. I think this is not the case, but again I may be overlooking something.
This might mean that we may need to create a new model for the /wp/v2/themes response along with its local persistence counterparts, which might require some more effort.
There was a problem hiding this comment.
Good point, I'm afraid that this could turn more complex than what we would expect for this task. At this point, I'd lean towards stick with using the fetch as you originally implemented and open an issue as a follow-up to tackle this in the future, wdyt?
There was a problem hiding this comment.
I agree, opened an issue for this: wordpress-mobile/gutenberg-mobile#3825
|
Size Change: -15 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
| // few popular ones. | ||
| ( category !== 'embed' || | ||
| ( category === 'embed' && | ||
| ALLOWED_EMBED_VARIATIONS.includes( id ) ) ) |
There was a problem hiding this comment.
Normally the embed variations were hidden from the inserter as we had an empty embed/variations.native.js file (which I deleted in this PR). We needed to re-enable those as the information if an embed supports this setting is kept in the attributes there with the responsive flag:
This should also help us when we add a few more variations for wordpress-mobile/gutenberg-mobile#3278
| @@ -118,7 +115,7 @@ const EmbedNoPreview = ( { label, icon, isSelected, onPress } ) => { | |||
| onPress={ onPressContainer } | |||
| > | |||
| <View style={ containerStyle }> | |||
| <Icon icon={ icon } fill={ embedIconStyle.fill } /> | |||
| <BlockIcon icon={ icon } /> | |||
There was a problem hiding this comment.
Now that we've enabled all the variations, icons can also be objects:
gutenberg/packages/block-library/src/embed/icons.js
Lines 26 to 35 in 3e77c78
BlockIcon component was used in the web version and it handles this case as well.
There was a problem hiding this comment.
I noticed the following issues when testing:
- After toggling off the "Resize for smaller devices" option, it's required to update the post before displaying the preview, otherwise, the embed is not displayed as expected. I think this behavior could be confusing to users, as they probably expect that modifications will be reflected in the preview without saving, wdyt?
- I tested with a post that contains all the providers and I noticed that the one for Amazon is not displaying the corresponding title and icon 🤔, any clue what is happening for this block?
| @@ -6,7 +6,7 @@ import apiFetch from '@wordpress/api-fetch'; | |||
|
|
|||
| // Please add only wp.org API paths here! | |||
| const SUPPORTED_ENDPOINTS = [ | |||
| /wp\/v2\/(media|categories|blocks)\/?\d*?.*/i, | |||
| /wp\/v2\/(media|categories|blocks|themes)\/?\d*?.*/i, | |||
There was a problem hiding this comment.
I'm wondering if we should fetch theme data via the bridge instead of directly making the API request, for other values like colors or gradients, we're getting them from the editor settings:
gutenberg/packages/react-native-bridge/ios/Gutenberg.swift
Lines 193 to 214 in 59530f3
From my POV it's totally ok to fetch from the editor but we might have issues for supporting offline mode, wdyt?
It eventually comes down to this: gutenberg/packages/block-library/src/embed/util.js Lines 268 to 273 in 70c6a8a We use the title before fetching the preview, so it's correct. But in the fetched preview from I'm not sure why but I observed that on WPCom simple sites this works fine, even when you add a generic embed block, if you use an |
I think it has to do with the API used in each case, on Atomic, I see that it's fetching the following URL: However, on WPcom, instead of hitting that endpoint it's directly getting the embed content with: Not sure how we could address this issue but as it's also happening in the web version, I'm wondering if we could tackle it separately and not block this PR, wdyt? |
I tested this PR on the inline preview one and I think I found the culprit for this. Looks like the Case 1 - Embed block is created ✅HTML code: Post preview: In this case, the preview is rendered properly with "Resize for smaller devices" option enabled. Case 2 - "Resize for smaller devices" option is disabled ❌HTML code:
Note that the In fact, if you switch to HTML code, switch back to visual mode and again to HTML code, you'll notice that the
This is the reason why it's required to update the post before displaying the preview, as it removes or empties the I haven't explored further but I have the impression that we would need to update the |
I agree that it could be tackled separately. Maybe we can add a special case for Amazon's |
That's an awesome find @fluiddot! I was looking into preview and remote draft update logic and was suspecting that it wasn't working as expected, it makes much more sense now. Thanks a lot 🙇 |
Yeah, we could consider Amazon's case as a special one but I'm concerned that we bump in similar cases in the future, I think it would be great if we investigate this further in case it's covering an unknown issue but in any case, I wouldn't block the PR as it can be replicated in the web version. |
| return ( | ||
| <SwitchCell | ||
| label={ label } | ||
| id={ id } | ||
| help={ help } | ||
| help={ helpLabel } |
There was a problem hiding this comment.
I noticed that on Android when tapping over the "Resize for smaller devices" element the ripple animation covers also the help message, not sure if this is expected 🤔 .
Screen_Recording_20210813-153056_WordPress.Pre-Alpha.mp4
There was a problem hiding this comment.
I think I didn't make changes around that in this PR. I've checked with Audio Block settings and it seems to be the same there. Pressing directly on the toggle doesn't create that effect, so I guess it could be the bottom sheet row being a touchable that's making that happen.
There was a problem hiding this comment.
Right, looks like this is related to the fact that the help message is rendered within the TouchableRipple component:
gutenberg/packages/components/src/mobile/bottom-sheet/cell.native.js
Lines 397 to 401 in b77066b
If we're using it in other places I guess it's expected, thanks for checking it 🙇 .
Description
If embed block variation and site's theme supports it, a settings icon is shown inside the block and a bottom sheet opens when pressing it, which shows a toggle for “Resize for smaller devices.”
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3753
How has this been tested?
Which embed variations support this setting can be checked from the
responsiveattribute invariations.js:Supported:
gutenberg/packages/block-library/src/embed/variations.js
Line 44 in 3e77c78
Unsupported:
gutenberg/packages/block-library/src/embed/variations.js
Line 338 in 3e77c78
For supported case:
For unsupported case:
A WP.com free plan site may not supportThis is actually not related to the site plan, but only related to which theme the site is using and if that theme supportsresponsive-embeds, so it can be used to test where the setting doesn't appear.responsive-embeds. For example,Maylandtheme supports responsive-embeds, butBaskerville 2doesn’t.Another way to test unsupported case is with an unsupported embed variation like
Amazon Kindleembed.Known issues
Facebook and Instagram embed variations have
blockin theirscopeas they are deprecated:gutenberg/packages/block-library/src/embed/variations.js
Line 66 in aa22cd7
This results in
responsiveattribute not being passed into the embed block and as a result it's assumedfalse.Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).