-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Implement specific Embed preview #33426
Conversation
# Conflicts: # package-lock.json # packages/block-library/src/embed/edit.js # packages/block-library/src/embed/embed-preview.native.js # packages/react-native-bridge/android/react-native-bridge/build.gradle # packages/react-native-editor/ios/Podfile.lock # packages/react-native-editor/package.json
Size Change: +1.34 kB (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
# Conflicts: # package-lock.json # packages/block-library/src/embed/edit.native.js # packages/block-library/src/embed/embed-preview.native.js # packages/react-native-editor/package.json
# Conflicts: # packages/react-native-bridge/android/settings.gradle
</head> | ||
<body | ||
data-resizable-iframe-connected="data-resizable-iframe-connected" | ||
className={ type } |
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 on web a few more class names are added to the type
here:
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 81 to 85 in c9612a1
const sandboxClassnames = classnames( | |
type, | |
className, | |
'wp-block-embed__wrapper' | |
); |
Should we replicate the same here?
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 might be overlooking something the logic in the web version but I checked the HTML of the iframes and there're no styles defined for these classes, maybe it's being used for other functionality.
In any case, I'll replicate this logic in the native version just in case.
EDIT: I added the sandbox classnames and it actually had an effect on the styles because in some cases it's adding the class wp-has-aspect-ratio
which does have styles:
gutenberg/packages/components/src/sandbox/index.js
Lines 85 to 91 in 8ef2008
html.wp-has-aspect-ratio, | |
body.wp-has-aspect-ratio, | |
body.wp-has-aspect-ratio > div, | |
body.wp-has-aspect-ratio > div iframe { | |
height: 100%; | |
overflow: hidden; /* If it has an aspect ratio, it shouldn't scroll. */ | |
} |
However, for some reason it's not rendering properly the previews:
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.
Following my previous comment, I'll investigate further the implications of the styles injected in the static HTML because I'm noticing discrepancies depending on the provider:
gutenberg/packages/components/src/sandbox/index.js
Lines 79 to 95 in 8ef2008
html, | |
body, | |
body > div, | |
body > div iframe { | |
width: 100%; | |
} | |
html.wp-has-aspect-ratio, | |
body.wp-has-aspect-ratio, | |
body.wp-has-aspect-ratio > div, | |
body.wp-has-aspect-ratio > div iframe { | |
height: 100%; | |
overflow: hidden; /* If it has an aspect ratio, it shouldn't scroll. */ | |
} | |
body > div > * { | |
margin-top: 0 !important; /* Has to have !important to override inline styles. */ | |
margin-bottom: 0 !important; | |
} |
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 styles.native.scss
may be missing some of the styles defined in the web version styles.scss
. Should we inject those directly into the webview 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.
Do you mean this style file from the sandbox component? If so, I don't think it's necessary because those styles are only used in the iframe component that is only rendered on the web version (reference).
In this commit I updated the generated styles and I verified with the changes from the block settings PR that it properly renders as expected when enabling/disabling the "Resize for smaller devices" option 🎊 .
RPReplay_Final1628269839.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.
Sorry, I meant the one from embed, but I guess they were not necessary seeing that it works fine now 🎉
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 did a few tests with the provided HTML and also by adding Embed blocks manually and this is working great so far. I've tested rotating, but I'm not sure how to test the resizing functionality.
I've commented with a few questions related to the preview and sandbox components, but overall I think it's looking very good. Thank you for the amazing work @fluiddot!
Resizing functionality can be tested when rotating the device because it needs to be recalculated, in any case, we'll also test it when using the "Resize for smaller devices" option in the block settings PR. |
@ceyhun The PR is ready for another review, so far I've spotted the following style issues but I think we could address them in a separate PRs: Preview's height is bigger than the content
Preview's height is smaller than the content and it gets cut off
Content's width doesn't fit the block
|
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've tested this with the builds for WPiOS(here) and WPAndroid(here). Found a few more style issues, but I don't think they are blockers.
- Vimeo had a height issue (for both iOS and Android) instead of width for me.
- Kickstarter had a height issue (for both iOS and Android), but it was with the webview itself, instead of the block. Also when I previewed the site page, it showed me a paragraph with the link instead of the embed.
- Facebook link showed me a blank embed block instead of an error message.
- For Twitter do you think it would be possible to keep showing the loading indicator until the webview is finished loading the URL? I think it might not seem very nice when it's loaded after a few moments. Again it's not a blocker, but maybe we can create a task to look into this if you think it's possible.
twitter.mp4
Thank you very much @ceyhun for the review ❤️ , I'll start the merge domino.
Ok, that makes sense, I'll add the style issues you found to the new issue I'm creating as a follow-up. EDIT: Here is the reference to the issue: wordpress-mobile/gutenberg-mobile#3823
Looks like it depends on the
Right, I managed to reproduce it by disabling the "Resize for smaller devices" option, however, I noticed that this issue is also present when previewing the post (see attached screenshot), so it might be related to the preview content 🤔.
Interesting, I didn't encounter that issue, could you share the URL you used?
Yep, we need to implement a similar error screen as we have on the web for this case (see attached screenshot). But I'm not sure if we should handle this case on the "Implement generic “No Preview” Embed preview UI" task or create a new issue, wdyt?
That would be great but in this case, it's a bit tricky because it's not enough listening for the WebView loading to finish. In fact, when it finishes, the content that is loaded is the one without styles but includes a JS script that transforms the content into tweet format in a second phase.
|
I noticed that the pods weren't updated in the demo project so I updated them in this commit. |
I used
I think “No Preview” UI is for cases when embed is successful, but the preview can only be shown when we preview the post itself. I'm not sure for which URL that would be like that though. For the Facebook case, when we preview the post, I think we still won't be able to see the embed.
Ah, in that case I guess we might need to leave it as it is for now :( |
I tried it on a local build and works. However, now that you mention it I remember I had a similar issue some days ago, but I don't know what was the cause. Let's keep this in mind when we do the final testing of the block, in case we still can reproduce it.
Ok I see, in that case, it doesn't make sense to attach the Facebook case to that state. We would need to check the following code to get some clues about which URL could lead to the "No Preview" case: gutenberg/packages/block-library/src/embed/edit.native.js Lines 79 to 88 in 8ff6687
In the web version, I noticed that the Facebook error is displayed here when gutenberg/packages/block-library/src/embed/embed-preview.js Lines 129 to 137 in e10962c
So we could include something similar under the gutenberg/packages/block-library/src/embed/embed-preview.native.js Lines 119 to 124 in e10962c
👍 |
gutenberg-mobile
PR: wordpress-mobile/gutenberg-mobile#3725WordPress-iOS
PR: wordpress-mobile/WordPress-iOS#16866WordPress-Android
PR: wordpress-mobile/WordPress-Android#15051Description
This PR includes the
react-native-webview
native module to render the HTML of the embed preview.Following the implementation of the web version, a native version of the
SandBox
component has been implemented. In the following sections, I briefly describe the key points of this component and how it has been addressed for the native version.Details
Embed preview component
Interactive flag and overlay
✅ It doesn't require further implementation.
Related code
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 45 to 52 in 105dc1d
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 101 to 108 in 105dc1d
This is mainly used to show/hide an overlay view, as far as I checked, the purpose of this logic is to prevent the user to interact with the preview unless the block and the Iframe are focused. In native, this is not necessary as we can easily control the interaction via the
pointerEvents
prop of theView
component and theisSelected
prop.Sandbox classnames
🔧 This will be addressed in this issue: wordpress-mobile/gutenberg-mobile#3284
Related code
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 81 to 85 in 105dc1d
gutenberg/packages/block-library/src/embed/embed-preview.js
Line 100 in 105dc1d
I'm not sure if they're required in native, but it would be worth investigating it, maybe we would need them for the alignment.
WpEmbedPreview
component🔧 This will be addressed in this issue: wordpress-mobile/gutenberg-mobile#3744.
Related code
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 92 to 93 in 105dc1d
This PR doesn't support yet WordPress embeds.
Not previewable case
🔧 This will be addressed in this issue: wordpress-mobile/gutenberg-mobile#3277
Related code
gutenberg/packages/block-library/src/embed/embed-preview.js
Lines 122 to 138 in 105dc1d
We're currently displaying the unavailable preview fallback, now that we support previews, we should update this component.
Sandbox component
FocusableIframe
component✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 241 to 249 in 105dc1d
This component is mainly used in the web version to determine if the Iframe has been focused and notify the embed preview component. In the embed preview, the focus event is used for controlling the preview interaction, as we're going to control it in a different way, implementing this component won't be necessary for the native version.
Resize JS observer
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 16 to 73 in 105dc1d
This JS code is used to listen for size changes within the preview, initially, we can use the same version in native. The only tweak I introduced is the object used to notify upstream, on native is
ReactNativeWebView
.gutenberg/packages/components/src/sandbox/index.native.js
Line 27 in 0d35c24
Iframe style
🔧 This will be addressed in this issue: wordpress-mobile/gutenberg-mobile#3284
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 75 to 96 in 105dc1d
This is the style object used as the base style for the preview. I noticed that it references the class name
wp-has-aspect-ratio
, we should check what's the purpose of this and if required implement it in the native version.isFrameAccessible
function✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 110 to 116 in 105dc1d
Not required because the webview component is always accessible.
trySandbox
functioncontentDocument
ref✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 123 to 124 in 105dc1d
gutenberg/packages/components/src/sandbox/index.js
Lines 170 to 175 in 105dc1d
It's used for injecting the HTML to the IFrame in the web version, initially, it won't be necessary for the native version as we use the
source
prop of thewebview
component.However, I noticed that the
body
property is extracted from this value so we should investigate if it's required.EDIT: This is not required in the mobile version.
gutenberg/packages/components/src/sandbox/index.js
Line 128 in 105dc1d
ownerDocument
ref - Language value🔧 Being addressed in
rnmobile/embed-block-preview-locale
branch.Related code
gutenberg/packages/components/src/sandbox/index.js
Line 139 in 105dc1d
Used to extract the language from the browser so it's not necessary for the native version. For now, in the native version, the language is fixed to
en
but we should fetch it from the device via the locale.gutenberg/packages/components/src/sandbox/index.native.js
Lines 111 to 112 in 0d35c24
data-resizable-iframe-connected
attribute✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 126 to 131 in 105dc1d
gutenberg/packages/components/src/sandbox/index.js
Line 153 in 105dc1d
gutenberg/packages/components/src/sandbox/index.js
Line 66 in 105dc1d
Looks like it's used to know if the resize observer has been initialized, we should investigate if it's required.
EDIT: This is not required in the mobile version.
HTML Doc
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 137 to 168 in 105dc1d
This is the HTML injected into the webview, the current code is ok but I included the following meta tag to fit the content:
gutenberg/packages/components/src/sandbox/index.native.js
Lines 122 to 125 in 0d35c24
Resize message handling
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 185 to 210 in 105dc1d
This is the callback used for handling resize events from the resize JS observer. I'd like to note that I removed a part that is used to verify that the origin of the message is the resize observer, in native, this is not really necessary as the resize message it's the only one that we're receiving.
Load event listener
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 215 to 219 in 105dc1d
The load event is used to call the
trySandbox
function after the load in the web version, as far as I checked, this is not required but it would be nice to investigate it further.EDIT: This is not required in the mobile version.
Message event listener
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Line 220 in 105dc1d
In this case, we use the webview component message system so it's not required.
IFrame extra props
✅ It doesn't require further implementation.
Related code
gutenberg/packages/components/src/sandbox/index.js
Lines 241 to 249 in 105dc1d
In the web version, we have the following extra props passed to the Iframe:
className="components-sandbox"
sandbox="allow-scripts allow-same-origin allow-presentation"
We should verify if they're required in the native version.
NOTE: The size in the native version is handled by passing an inline style with the aspect ratio calculated from the preview size.
gutenberg/packages/components/src/sandbox/index.native.js
Lines 188 to 190 in 0d35c24
EDIT: This is not required in the mobile version.
How has this been tested?
Test all providers
HTML code
NOTE: The following providers are not working on both platforms:
Interaction is disabled
Inline preview disabled in production mode
Performance
Preparation:
Opening a post/page
Scrolling content
Screenshots
embed-block-preview-ios.mp4
embed-block-preview-android.mp4
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).