[RNMobile] Embed block - Link in Block Settings #36099
Conversation
jd-alexander
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.09 MB ℹ️ View Unchanged
|
| bottomSheetLabel={ bottomSheetLabel } | ||
| url={ url } | ||
| onEmbedBottomSheetSubmit={ ( value ) => { | ||
| // The order of the following calls is important, we need to update the URL attribute before changing `isEditingURL`, | ||
| // otherwise the side-effect that potentially replaces the block when updating the local state won't use the new URL | ||
| // for creating the new block. | ||
| setAttributes( { url: value } ); | ||
| setIsEditingURL( false ); | ||
| } } | ||
| onEmbedBottomSheetClose={ () => | ||
| setShowEmbedBottomSheet( false ) | ||
| } | ||
| showEmbedBottomSheet={ showEmbedBottomSheet } |
There was a problem hiding this comment.
These props are similar to how the EmbedBottomSheet is used within this Component. Let me know if the logic sharing approach here is fine or if there's a more optimal way to do this.
There was a problem hiding this comment.
Now that the visibility of EmbedBottomSheet component is controlled by EmbedControls, probably we could render EmbedBottomSheet within the EmbedControls component and only expose here a prop to notify when the URL is edited, wdyt?
There was a problem hiding this comment.
@fluiddot I think that approach is fine. Do we want to link picking behavior to be the same for both adding and editing a link? I assume that would make the experience more optimal. However, I noticed that when adding a link the embed bottom sheet would accept the input directly but when editing a link as done here, there would be a picker for that link because that is the approach that I see being utilized in other Block Settings. So the question is do we utilize inline editing instead of a Picker for both experiences? I think doing this would allow the experience to be optimal. ( At first, I had tried this but I think how the attributes were being set was possibly causing an issue so I didn't pursue it)
Now when I think about it, I agree with the case where if we utilize a prop to notify when a URL is being edited then, only at that time would we need to render the link editing component within the BlockSettings PanelBody otherwise we return the bottom sheet. i.e a function that does something similar to the link settings logic below.
There was a problem hiding this comment.
@fluiddot I think that approach is fine. Do we want to link picking behavior to be the same for both adding and editing a link? I assume that would make the experience more optimal. However, I noticed that when adding a link the embed bottom sheet would accept the input directly but when editing a link as done here, there would be a picker for that link because that is the approach that I see being utilized in other Block Settings. So the question is do we utilize inline editing instead of a Picker for both experiences? I think doing this would allow the experience to be optimal. ( At first, I had tried this but I think how the attributes were being set was possibly causing an issue so I didn't pursue it)
I think it makes sense to provide the same experience in both cases. I'd stick with the option where the user can edit directly the URL (as we're currently providing). The one that presents the picker can be confusing because it's related to linking the block to an URL, but in this case, we're editing the source of the embed.
Now when I think about it, I agree with the case where if we utilize a prop to notify when a URL is being edited then, only at that time would we need to render the link editing component within the BlockSettings PanelBody otherwise we return the bottom sheet. i.e a function that does something similar to the link settings logic below.
Yep, exactly. I hope this doesn't introduce undesired side effects but looks like a promising way to go 👍 . Let me know if I can help anyhow in the implementation 🙇 .
There was a problem hiding this comment.
I think it makes sense to provide the same experience in both cases. I'd stick with the option where the user can edit directly the URL (as we're currently providing). The one that presents the picker can be confusing because it's related to linking the block to an URL, but in this case, we're editing the source of the embed.
Agreed!
Yep, exactly. I hope this doesn't introduce undesired side effects but looks like a promising way to go 👍 . Let me know if I can help anyhow in the implementation 🙇 .
Thanks for confirming 🙇🏾 My goal was to take a look at it after I am done WPiOS testing efforts and crash monitoring. If you would want to do the refactor and it doesn't take much effort from your side I am more than happy to review the changes afterward in an effort to keep things moving.
There was a problem hiding this comment.
Thanks for confirming 🙇🏾 My goal was to take a look at it after I am done WPiOS testing efforts and crash monitoring. If you would want to do the refactor and it doesn't take much effort from your side I am more than happy to review the changes afterward in an effort to keep things moving.
I'd like first to wrap up first the tests tasks but after that, I'll be more than happy to help with this implementation. I'll let you know if I start tackling it 👍 .
jd-alexander
changed the title
There was a problem hiding this comment.
Thanks @jd-alexander for tackling this ❤️!
I've done a quick review and I'd like to propose a different approach where the EmbedBottomSheet would be rendered within the EmbedControls. I think it would be possible but I might be overlooking a potential blocker, let me know what you think, I'll be more than happy to help to land it 🙇 .
| bottomSheetLabel={ bottomSheetLabel } | ||
| url={ url } | ||
| onEmbedBottomSheetSubmit={ ( value ) => { | ||
| // The order of the following calls is important, we need to update the URL attribute before changing `isEditingURL`, | ||
| // otherwise the side-effect that potentially replaces the block when updating the local state won't use the new URL | ||
| // for creating the new block. | ||
| setAttributes( { url: value } ); | ||
| setIsEditingURL( false ); | ||
| } } | ||
| onEmbedBottomSheetClose={ () => | ||
| setShowEmbedBottomSheet( false ) | ||
| } | ||
| showEmbedBottomSheet={ showEmbedBottomSheet } |
There was a problem hiding this comment.
Now that the visibility of EmbedBottomSheet component is controlled by EmbedControls, probably we could render EmbedBottomSheet within the EmbedControls component and only expose here a prop to notify when the URL is edited, wdyt?
|
@jd-alexander regarding the issue about the invalid links not leading to a validation error, what URLs did you use for testing? I recall I used the value |
For this particular case, I didn't use a URL, I had just typed in a random set of letters. However, this error only occurs when the |
Ok, I'm glad to know that it's not happening in the inline editing experience. In any case, it would be great to double-check this once we finish the implementation, thanks for clarifying it 🙇 . |
|
@jd-alexander I'm going to devote some time today to implement the approach we discussed in this comment. |
# Conflicts: # packages/block-library/src/embed/embed-bottom-sheet.native.js # packages/react-native-editor/CHANGELOG.md
The EmbedBottomSheet component has been also renamed to EmbedLinkSettings because the component is no longer strictly tied to the bottom sheet component.
|
@jd-alexander Thank you very much for starting addressing the issues on the integration tests 🥇!
Right, there are two instances because the component In this case, we can solve the issue by using the
In general, it shouldn't be necessary to wait for visibility changes in the bottom sheet, however in this case, as we use the dismiss action to perform editing the URL we need it. Apart from introducing the use of
@jd-alexander I think the PR is finally ready for a final review, let me know if you could take a look, thanks 🙇 . |
|
I noticed that the Performance and End-to-End Tests are also failing in |
|
@jd-alexander I updated the test cases described in the PR description due to the recent changes and added one for verifying that it allows editing the link after setting an invalid link. Additionally, I added a couple of integration tests: |
There was a problem hiding this comment.
I run all test cases and everything is working nice 🎊 , I only found a minor issue related to displaying the invalid URL error notice twice, here are the reproduction steps:
- Add an embed block.
- Set a valid URL.
- Open block settings.
- Change embed link to an invalid format URL (example:
http://). - Tap on the enter key on the keyboard instead of dismissing the bottom sheet.
- Observe that the
Invalid URL. Please enter a valid URL.error message is displayed twice.
embed-block-edit-link-error-notice-duplicated.mp4
After debugging this issue, I noticed that the performOnCloseOperations handler is being called twice when editing the URL by tapping on the enter key. However, when dismissing the bottom sheet is only called once 🙃 .
packages/block-editor/src/components/block-settings/container.native.js
Outdated
Show resolved
Hide resolved
Thanks for spotting this error @fluiddot I resolved this issue by storing if the bottom sheet's close event was consumed in an instance variable via The reason this occurs is because Let me know if this approach is optimal. Thank you 🙇🏾 |
Thanks! The tests are looking good 👍🏾 |
@jd-alexander thank you very much for addressing the issue ❤️ ! I've just reviewed the changes and provided a change suggestion in this comment. |
|
I noticed that the CI check However, this test is also failing in the commits in |
There was a problem hiding this comment.
LGTM 🎊 ! Awesome work @jd-alexander 🥇 !
I've done the following test cases and verified that they passed 🟢 :
- Add Embed block - bottom sheet functionality remains ✅
- Edit Embed block - change the link in Block Settings ✅
- Edit Embed block - Invalid format link does not lead to validation error ✅
- Edit Embed block - Invalid link ✅
Tested in iPhone 12 Pro Max simulator (iOS 15.0) and Samsung Galaxy S20 FE 5G (Android 11).
The |
Thanks for the review and tests @fluiddot 🙇🏾 |
Gutenberg-MobilePR wordpress-mobile/gutenberg-mobile#4189Description
This PR is ready for a first pass while some minor issues are investigated 🙇🏾
This PR introduces the Link in the Block Settings of each Embed block. Since the Link is now within the Block Settings component, we no longer need to display the Pencil icon ( Edit action) with the Toolbar, therefore this functionality was removed. To facilitate these changes, the
embed-controls.jscomponent that was shared between the web and mobile, was duplicated and modifications were made to theembed-controls.native.jsvariant to support the Block Settings and Toolbar behavior specific to mobile as described in the associated GB-Mobile ticket.How has this been tested?
Add Embed block - bottom sheet functionality remains ✅
The main purpose of this test case is to ensure that the changes made here did not impact the existing functionality.
Screen.Recording.2021-11-02.at.1.14.21.AM.mov
Edit Embed block - change the link in Block Settings ✅
embed-block-edit-link.mp4
Edit Embed block - Invalid format link does not lead to validation error 🟢
http://).Since the Pencil icon is no longer present we are now left in a state where the link cannot be edited.I think this occurs because the
LinkPickeraddshttp://to all entries hence theembed-bottomsheetlogic believes the URL is true. I will investigate this further.gutenberg/packages/block-library/src/embed/embed-bottom-sheet.native.js
Line 51 in 3e371ca
embed-block-edit-link-invalid.mp4
Edit Embed block - Invalid link 🟢
https://example.com).embed-block-edit-link-retry.mp4
Types of changes
EmbedControlwas created.Checklist:
*.native.jsfiles for terms that need renaming or removal).