[RNMobile] Embed Block caption field #32226
Merged
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
jd-alexander
added
Mobile App - i.e. Android or iOS
jd-alexander
changed the title
jd-alexander
changed the title
2 tasks
|
Size Change: +8 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
fluiddot
reviewed
Jun 30, 2021
Comment on lines
196
to
198
| const showEmbedPlaceholder = ! preview || cannotEmbed || isEditingURL; | ||
| //const showEmbedPlaceholder = ! preview || cannotEmbed || isEditingURL; | ||
| const showEmbedPlaceholder = false; |
There was a problem hiding this comment.
We have to hide the embed placeholder to test the PR but I'd revert this change once it's approved.
fluiddot
reviewed
Jun 30, 2021
Comment on lines
62
to
67
| <View style={ styles[ 'embed-preview__placeholder' ] }> | ||
| <Text style={ styles[ 'embed-preview__placeholder-text' ] }> | ||
| Embed Preview will be directly above the Block Caption | ||
| component when it is implemented. | ||
| </Text> | ||
| </View> |
There was a problem hiding this comment.
This placeholder will be removed once we include the preview content which will be addressed in this issue:
wordpress-mobile/gutenberg-mobile#3275.
fluiddot
reviewed
Jun 30, 2021
|
@hypest The PR is ready to review, most of the work was already done so I only applied some improvements and conducted testing. So far everything looks fine but I'd like to note that there're a couple of code blocks that have been included only for testing purposes (described in the PR description), once the PR is approved we should remove them before merging. |
|
Renders properly ✅ Verify that unselecting the Embed block causes the empty caption to disappear. HTML output is valid ✅ Add an Embed block. |
hypest
reviewed
Jul 1, 2021
Good catch! I'll review this part on Android, we're using the |
tbh, I don't feel that's enough of a blocker for this PR, if the caption doesn't get selected. I might even argue that it shouldn't be selected upon adding a new embed block since the caption is not the primary interaction entry point for this particular block (the URL is). |
hypest
approved these changes
Jul 1, 2021
There was a problem hiding this comment.
LGTM! Awesome work here @jd-alexander and @fluiddot !
Feel free to remove the testing code if you want, before merging.
fluiddot
force-pushed
the
rnmobile/embed-block-caption
branch
from
744939c to
9f14b7e
Compare
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
This PR implements the Embed Block caption functionality.
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3552
gutenberg/packages/block-library/src/embed/edit.js
Lines 87 to 92 in 93def61
How has this been tested?
Note: Currently, the
EmbedPreviewis not available so aTextcomponent simulates it's position within the container. The Embed Preview will be implemented here wordpress-mobile/gutenberg-mobile#3276Renders properly
Embed Placeholderis disabled so theEmbedPreviewcan be rendered without setting a link. ( Link setting was implemented in Embed Block Bottom Sheet to set a link wordpress-mobile/gutenberg-mobile#3436RichTextcomponent to lose focus.HTML output is valid
Embed Placeholderis disabled so theEmbedPreviewcan be rendered without setting a link. ( Link setting was implemented in Embed Block Bottom Sheet to set a link wordpress-mobile/gutenberg-mobile#3436<br>on line-breaks.Screenshots
iOS Light Mode
iOS Dark Mode
Android Light Mode
Android Dark Mode
Types of changes
EmbedPreviewcomponent was recreated here even though there's a WIP of it in [Embed block] Fetching embed preview and loading UI wordpress-mobile/gutenberg-mobile#3275 this was done for demonstration purposes.Checklist:
*.native.jsfiles for terms that need renaming or removal).