[RNMobile][Embed block] Fetching embed preview and loading UI #33260
Conversation
This file contains 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
ceyhun
added
the
Mobile App - i.e. Android or iOS
2 tasks
|
Size Change: +9 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
Merged
4 tasks
Merged
3 tasks
fluiddot
reviewed
Jul 7, 2021
Comment on lines
+22
to
+27
| const activityIndicatorColor = Platform.select( { | ||
| ios: '#999999', // default color on iOS | ||
| // ActivityIndicator is blank and does not show up unless explicit color prop is provided. | ||
| // See https://github.com/facebook/react-native/pull/30057 | ||
| android: PlatformColor( '?attr/colorControlActivated' ), | ||
| } ); |
There was a problem hiding this comment.
Not for this PR but I'm thinking that it would be nice if we could have this component generic because we most likely require it in future components.
There was a problem hiding this comment.
That's a good idea, other places where ActivityIndicator is used could have the same issue on Android already. I've created an issue for this: wordpress-mobile/gutenberg-mobile#3708
Comment on lines
+68
to
+85
| { cannotEmbed ? ( | ||
| <> | ||
| <Text style={ labelStyle }> | ||
| { __( | ||
| 'Sorry, this content could not be embedded.' | ||
| ) } | ||
| </Text> | ||
| <Text | ||
| style={ styles[ 'embed-empty__description' ] } | ||
| > | ||
| { __( 'EDIT LINK' ) } | ||
| </Text> | ||
| </> | ||
| ) : ( | ||
| <Text style={ styles[ 'embed-empty__description' ] }> | ||
| { __( 'ADD LINK' ) } | ||
| </Text> | ||
| ) } |
There was a problem hiding this comment.
In an incoming PR, I will replace this part with a new component EmbedNoPreview.
| ) : ( | ||
| <Image | ||
| style={ { | ||
| flex: 1, |
There was a problem hiding this comment.
What do you think about moving this to a style object?
There was a problem hiding this comment.
The aspectRatio is already dynamically generated so we'll need to pass that in from code again and only flex: 1 will be in a style object, so maybe we can skip it for now?
fluiddot
reviewed
Jul 7, 2021
There was a problem hiding this comment.
Tested on iPhone 11 (iOS 14.2) and Samsung Galaxy S20 FE 5G (Android 10).
I tested the following cases in a free WP.com site, Atomic site, and self-hosted without Jetpack:
- An invalid URL
- A valid but unsupported embed URL (supported ones listed here)
- A valid embed URL with thumbnail
- A valid embed URL without thumbnail
All of them passed ✅ , the only thing I noticed is that the first time the preview is loaded it does a bit of flickering, see attached video capture around 0:03:
embed-block-loading-flickering.mp4
The only comment that we could tackle is this one but we might postpone it as we most likely update that part if we want to implement styles there for the alignment options, wdyt?
Thanks for the review! I agree that we can update it later if necessary 👍 I'll see if we can find a quick fix for the flickering, but otherwise I think we can leave it for later as well. |
7 tasks
If we address the flickering in a different then this PR LGTM 🎊 ! |
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
Porting Embed block to mobile Gutenberg, here we implemented:
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3704
How has this been tested?
Add an Embed block
In the bottom sheet try entering:
Try with:
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).