[RNMobile] Embed block - Set EmbedLinkSettings URL input cursor at start of link #36100
Conversation
This file contains hidden or 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
the
Mobile App - i.e. Android or iOS
jd-alexander
changed the title
|
Size Change: 0 B Total Size: 1.08 MB ℹ️ View Unchanged
|
jd-alexander
changed the title
2 tasks
fluiddot
reviewed
Nov 5, 2021
Contributor
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
The functionality works great but I'm wondering if this behavior might be confusing to the user. On iOS, the cursor is always placed at the beginning of the URL when is being edited, which is not a common behavior if the text input is already populated. Besides, after checking the original issue I understand that the approach proposed was to only do this in case the URL is auto pasted from the clipboard:
Additionally we might want to consider placing the cursor of an auto pasted url to the left of the text so the domain name is clear.
So I'm wondering if we should limit this behavior to that specific case, wdyt?
Contributor
Author
Yes, it does make sense to limit the behavior to auto-pasted URLs as that would help to avoid any confusion that would be present and this customization would be limited to the use case in the linked issue. I will look at a mechanism for querying the state of the |
fluiddot
reviewed
Nov 9, 2021
Contributor
fluiddot
left a comment
fluiddot
left a comment
There was a problem hiding this comment.
I noticed that the cursor is not set to start in the following case:
- Add an embed block.
- Set a valid embed URL and dismiss the bottom sheet.
- Open the block settings.
- Set an empty URL and dismiss the bottom sheet.
- Copy an URL to the clipboard.
- Tap on the empty block to display the edit URL bottom sheet.
- Observe that the input cursor is not at the start.
Most likely this issue is related to the fact that the setCursorAtStart state is only set when mounting the cell component.
| // * placeholder - control component placeholder, e.g. `Add URL` | ||
| // * autoFocus (url only) - whether url input should be focused on sheet opening | ||
| // * autoFill (url only) - whether url input should be filled with url from clipboard | ||
| // * setCursorAtStart (url only) - whether url input's selection should be set to the start. The cursor will be at the front of the link. |
Contributor
There was a problem hiding this comment.
Now that the cursor is only set to the start only when pasting a URL from the clipboard, it would be nice if we update this description and reflect that nuance. Besides, I'm wondering if we should rename the property's name, although I'd try to avoid making it super long 😅 .
Contributor
Author
There was a problem hiding this comment.
That is a good suggestion @fluiddot Thanks for spotting 🙇🏾♂️
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.
Fixes #35888
Gutenberg-Mobile PRwordpress-mobile/gutenberg-mobile#4211Description
This change introduces logic to the Embed link input that allows the cursor to be set at the start of the link so that the domain is visible. Currently, this functionality is restricted to iOS based on the description of the associated issue and the fact that
autoFocusis restricted to iOS.How has this been tested?
iOS
Screen.Recording.2021-11-05.at.12.40.54.AM.mov
Android
autoFocusenabled.Screen.Recording.2021-11-05.at.12.49.50.AM.mov
Types of changes
EmbedBottomSheetlinkSettingsOptionswas extended to include thesetCursorAtStartoption. This value is set withtruefor the iOS platform since it requiresautoFocusto work.Cellcomponent was extended to include thesetCursorAtStartprop has a means of setting theselectionof theTextInputcomponent to the start of the link so that the domain is visible.Concerns
link-settings. eg. Image Block. Doing that would make the behavior consistent. However, I opted not to do this since the associated ticket is focusing on the Embed block and I think seeing the domain name of the provider in the case of the Embed block provides more context to verify the link than when an Image is added. I am not sure it would benefit the Image block.TextInputwas being impacted because it looked a bit off. However, I think the behavior may be related to the fact that theTextInputis right-aligned. I was thinking this would be the case sincesetCursorStartis set tofalseonce the user starts editing the link.Checklist:
*.native.jsfiles for terms that need renaming or removal).