Skip to content
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

Update caret position after inserting link #716

Merged
merged 10 commits into from
Mar 13, 2019

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Mar 7, 2019

To Fix: #679

Gutenberg PR: WordPress/gutenberg#14317

With this PR we are putting the caret at the end of the linked text when we insert a link. This needs to be tested both on Android & iOS.

Following screen captures are taken from this PRs branch:

iOS:

link-ios

Android:

link-android

TO TEST
Testing prerequisites

For WPiOS

Checkout the PRs branch to any arbitrary folder and cd .. to it
run yarn install, yarn start
Open XCode WPiOS on the latest develop
Clean build folder on Xcode, and then run the app

For WPAndroid

open grade.properties at WordPress-Android folder
add wp.BUILD_GUTENBERG_FROM_SOURCE = true to grade.properties
checkout the PRs branch in the subrepo of WordPress-Android repo
cd to WordPress-Android/libs/gutenberg-mobile
run yarn install, yarn start
yarn wpandroid on a separate terminal in the same directory

Test steps:

Test 1 - Inserting link when no text is selected

  • Tap somewhere in a paragraph/heading block
  • Tap links button in the toolbar
  • Add a link and close the links ui
  • Verify that the caret is at the end of the inserted link

Test 2 - Editing an existing link

  • Select an existing link in a paragraph/heading block
  • Tap links button in the toolbar
  • Update the visual text of the link and close the links ui
  • Verify that the caret is at the end of the updated link

Test 3 - Select a text and add a link

  • Select some text in a paragraph/heading block
  • Tap links button in the toolbar
  • Add a url for the link and close the links ui
  • Verify that the selection remains same

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on Andorid, works like a charm LGTM :shipit:

@pinarol
Copy link
Contributor Author

pinarol commented Mar 8, 2019

@SergioEstevao could you please check for the iOS side? Thanks!

@hypest
Copy link
Contributor

hypest commented Mar 8, 2019

Sorry for the late input on this but, I'm wondering if the behavior implemented here will match the web's side behavior and whether any fixes should be done on the web side too. What do you think @pinarol ?

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working correctly on iOS. Good work!

@pinarol
Copy link
Contributor Author

pinarol commented Mar 8, 2019

@hypest 👋 I checked the convo for the same issue on for the web side, I based my fix on that one #679 (comment) I hope I didn't miss any other thing

@pinarol
Copy link
Contributor Author

pinarol commented Mar 8, 2019

just adding the related comment for the web side WordPress/gutenberg#7980 (comment)

@pinarol
Copy link
Contributor Author

pinarol commented Mar 8, 2019

hey @hypest 👋I did some investigation about how to share the caret positioning logic after inserting a link between web and mobile.

  • We have different submitLink functions for web and mobile and it looks like we can't use the same function for both. but we can call same sub-functions for parts that calculate the caret position, although this doesn't guarantee that platforms won't differ in the future but still better than nothing.

  • So in terms of sub-functions we already use the same insert() function, that means we already share the caret logic in cases where we insert a link with no text selected beforehands or update the existing link. For this case all I had to do was just reflecting the already calculated caret position to aztec and the problem was fixed.

  • The difference exists for the case where we select a text and transform it to a link. We use applyFormat method to achieve this and that method doesn't change the selection position so I am changing it afterwards on mobile side and web is not changing it yet. (what is more, we have apply-format.js, apply-format.native.js so they are different functions with different input params) Currently I couldn't come up with a nice solution to make both platforms use same logic for this case. we kinda need a new function like apply format and put caret at the end and call it from both web and mobile but it wouldn't serve the purpose if one day web just replaces that function call on their side. so I kinda ran out of ideas on this. I can really use some help or other ideas, thanks!

@pinarol pinarol modified the milestones: Future, v1.2 Mar 11, 2019
@hypest
Copy link
Contributor

hypest commented Mar 12, 2019

Thanks for the analysis @pinarol . Seems like we prbably need to break up some functions to smaller ones so that we can keep the cross-platform parts the same and only nativize the rest. Can you seek out the person working on it on the web side (if someone is already) or come up with a proposal (PR even) we can run by our web side friends?

@pinarol
Copy link
Contributor Author

pinarol commented Mar 12, 2019

@hypest as we discussed today I've opened an issue to do the follow up later since there's still an ongoing convo on web side. #737

I reverted the line which makes mobile behavior differ from web, currently web and mobile behaves same. Also updated the 'Test 3 - Select a text and add a link' in the description

@pinarol
Copy link
Contributor Author

pinarol commented Mar 12, 2019

hey @mzorz could you be able to check this again? I made a small change and updated Test 3, also updated the branch from develop.

@pinarol pinarol requested a review from mzorz March 12, 2019 17:48
@mzorz
Copy link
Contributor

mzorz commented Mar 12, 2019

Tested! Verified all 3 test cases work correctly. I found one weird case by mistake:

  1. start a draft, enter some text
  2. place the cursor on a word
  3. tap the link button
  4. in the URL field, insert something with a space in it, for example yahoo com instead of yahoo.com, and dismiss the links ui
  5. observe 2 different words are inserted where the cursor was, yahoo and com
  6. place the cursor on yahoo and tap the links button -> observe the links ui shows empty
  7. place the cursor on com and tap the links button -> observe the yahoo com (invalid) address appears there.

I think it would be wise to do some URL validation if we are not doing it? And if we are, then something's not working well there :)

I understand this is not specific to this PR which only cares about caret positioning so, I'm going to give this one the go, and open another issue for this.

EDIT: reported here #740

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

@pinarol pinarol merged commit a2342d4 into develop Mar 13, 2019
@pinarol pinarol deleted the issue/679-cursor-after-link branch March 13, 2019 06:32
daniloercoli added a commit that referenced this pull request Mar 13, 2019
…rg-mobile into issue/707-Placeholder-is-missing-on-Heading-block

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Update caret position after inserting link (#716)

# Conflicts:
#	gutenberg
daniloercoli added a commit that referenced this pull request Mar 15, 2019
…rg-mobile into issue/345-fix-for-press-delete-no-merge-move-focus

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (39 commits)
  Update gb hash after merging the companion PR
  Update GB ref
  Update gutenberg ref
  Update gutenberg ref
  Update gutenberg ref
  Tests: Add mock for the newly added CSS class
  Update GB ref
  Raise the minHeight value for heading block to 60 (it was 50)
  Introduce a constant for the string `tag` and use it instead of the hard coded string
  Make `tagName` private
  Update heading placeholder size on heading level change (#742)
  Fix - HTML Editor - Keyboard can not be closed (#743)
  Update gutenberg ref
  Update gutenberg ref
  Update GB ref
  Add comments
  Update caret position after inserting link (#716)
  Update GB hash
  Reuse `blockType` instead of adding a new property
  Update GB ref
  ...

# Conflicts:
#	src/block-management/block-holder.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link: After adding link, cursor moves to start of linked text
4 participants