-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[RNMobile][Embed block] Block settings #33654
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getThemeSupports
selector may trigger a request to /wp/v2/themes?status=active
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 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
private func properties(from editorSettings: GutenbergEditorSettings?) -> [String : Any] { | |
var settingsUpdates = [String : Any]() | |
settingsUpdates["isFSETheme"] = editorSettings?.isFSETheme ?? false | |
if let rawStyles = editorSettings?.rawStyles { | |
settingsUpdates["rawStyles"] = rawStyles | |
} | |
if let rawFeatures = editorSettings?.rawFeatures { | |
settingsUpdates["rawFeatures"] = rawFeatures | |
} | |
if let colors = editorSettings?.colors { | |
settingsUpdates["colors"] = colors | |
} | |
if let gradients = editorSettings?.gradients { | |
settingsUpdates["gradients"] = gradients | |
} | |
return settingsUpdates | |
} |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theresponsive-embeds
value 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
attributes: { providerNameSlug: 'twitter', responsive: true }, |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
export const embedTwitterIcon = { | |
foreground: '#1da1f2', | |
src: ( | |
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"> | |
<G> | |
<Path d="M22.23 5.924c-.736.326-1.527.547-2.357.646.847-.508 1.498-1.312 1.804-2.27-.793.47-1.67.812-2.606.996C18.325 4.498 17.258 4 16.078 4c-2.266 0-4.103 1.837-4.103 4.103 0 .322.036.635.106.935-3.41-.17-6.433-1.804-8.457-4.287-.353.607-.556 1.312-.556 2.064 0 1.424.724 2.68 1.825 3.415-.673-.022-1.305-.207-1.86-.514v.052c0 1.988 1.415 3.647 3.293 4.023-.344.095-.707.145-1.08.145-.265 0-.522-.026-.773-.074.522 1.63 2.038 2.817 3.833 2.85-1.404 1.1-3.174 1.757-5.096 1.757-.332 0-.66-.02-.98-.057 1.816 1.164 3.973 1.843 6.29 1.843 7.547 0 11.675-6.252 11.675-11.675 0-.178-.004-.355-.012-.53.802-.578 1.497-1.3 2.047-2.124z"></Path> | |
</G> | |
</SVG> | |
), | |
}; |
BlockIcon
component was used in the web version and it handles this case as well.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
private func properties(from editorSettings: GutenbergEditorSettings?) -> [String : Any] { | |
var settingsUpdates = [String : Any]() | |
settingsUpdates["isFSETheme"] = editorSettings?.isFSETheme ?? false | |
if let rawStyles = editorSettings?.rawStyles { | |
settingsUpdates["rawStyles"] = rawStyles | |
} | |
if let rawFeatures = editorSettings?.rawFeatures { | |
settingsUpdates["rawFeatures"] = rawFeatures | |
} | |
if let colors = editorSettings?.colors { | |
settingsUpdates["colors"] = colors | |
} | |
if let gradients = editorSettings?.gradients { | |
settingsUpdates["gradients"] = gradients | |
} | |
return settingsUpdates | |
} |
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: 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. |
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.
LGTM 🎊 !
Tested on Simulator iPad Pro (12.9-inch) - 4th generation (iOS 14.4) and Samsung Galaxy S20 FE 5G (Android 10).
return ( | ||
<SwitchCell | ||
label={ label } | ||
id={ id } | ||
help={ help } | ||
help={ helpLabel } |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
{ help && ( | |
<Text style={ [ cellHelpStyle, styles.placeholderColor ] }> | |
{ help } | |
</Text> | |
) } |
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
responsive
attribute 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,Mayland
theme supports responsive-embeds, butBaskerville 2
doesn’t.Another way to test unsupported case is with an unsupported embed variation like
Amazon Kindle
embed.Known issues
Facebook and Instagram embed variations have
block
in theirscope
as they are deprecated:gutenberg/packages/block-library/src/embed/variations.js
Line 66 in aa22cd7
This results in
responsive
attribute 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.js
files for terms that need renaming or removal).