Feature/set remove link interface android

[marecar3]

This PR is inspired by iOS implementation which was described in this PR.

In the branch gutenberg/rnmobile/test-links you can find a testing code, and see how it looks:
WordPress/gutenberg@mobile...rnmobile/test-links

[mzorz]

While testing this one I found a weird behavior:

Steps to reproduce:

1. select a word
2. tap on the link formatter icon
3. observe the link is created
4. tap on the link icon again and observe the url format is undone
5. deselect the word and place then cursor somewhere in the middle of the word
6. tap on the link, observe a "wp.com" link-formatted string is inserted.

Is that expected?
I believe it's happening because of the test code here WordPress/gutenberg@mobile...rnmobile/test-links#diff-ed195425f515f40db41c23d586760ef9R339 but I wonder whether this is something that you may want to look into for the code in react-native-aztec wrapper, so reporting here @marecar3 .

[etoledom]

Since we are inserting a link without selection and without title, this might be expected.

I don't recall what was the behavior on iOS for this specific case, but if it's different than Android, we might want to make them behave in the same way, whatever we decide is expected.

[marecar3]

Hey @mzorz , @etoledom I was inspired with iOS proposal so as @etoledom said

Since we are inserting a link without selection and without title, this might be expected.

It's a hardcoded string for now, while we are waiting for RN implementation.

[marecar3]

Hey @daniloercoli ,
I would be happy to also get some feedback from you when you get better, thanks! :)

[etoledom]

I don't recall what was the behavior on iOS for this specific case

I just tested it, and trying to add a link without selection, it just fails. It doesn't add the URL and the next typed characters doesn't have the link attributes.

I think that @Tug could tell us what behavior is more convenient for the implementation of the UI?

Giving the case that the user doesn't have anything selected, tries to add a link with the UI, and they just add the URL and not a "title" (or phrase/word to "hold" the link). It should probably add the url as that phrase/word.

The difference would be if the UI handler calls setLink(url, url) or setLink(url, null) (being the second parameter the word to be holding the url attribute).

I hope this makes sense.

[Tug]

@etoledom Actually we won't need setLink and removeLink from Aztec for the Link feature for now :(

Currently, RichText updates Aztec on every keystroke since we've removed the check in shouldComponentUpdate. This means we can do everything from the Gutenberg's side using its formatting library and makes a much cleaner/simpler approach. @koke opened a PR to do that WordPress/gutenberg#12249 some time ago and I just took over, rebased and made it work for Links, then added a simple UI ontop of that.

What happens now is that when the link is generated from gutenberg's side, the resulting HTML is sent back to Aztec which displays a link.

[marecar3]

Final conclusion from @Tug

So I think we don’t need it for now, at some point, when we work on performance, we might want to handle formatting on the Aztec side, but for now I think it’s better to reuse as much code from gutenberg as possible

That said we can still merge #99 if it works

cc: @etoledom

[etoledom]

cleaner/simpler approaches are always welcome! 😁
Thanks for the heads up @Tug !

Do you still need the link format message from native to show the "link" button selected on the toolbar?
And a way to extract the url information to be shown in the UI?

[Tug]

Do you still need the link format message from native to show the "link" button selected on the toolbar?

Nop, with #12249 we're removing the onActiveFormatsChange handler: https://github.com/WordPress/gutenberg/pull/12249/files#diff-ed195425f515f40db41c23d586760ef9L375

And a way to extract the url information to be shown in the UI?

We don't need it either, this is handled by gutenberg's format-library as well

[hypest]

We've folded this repo into gutenberg-mobile so, if this PR is still active please, migrate it to a PR on the gutenberg-mobile repo. Thanks!

[marecar3]

Hey @hypest, sorry for the late response.
Not sure what to do with this one, as @Tug manage to implement this logic on the RN side, but on the other hand, iOS PR with this changes is merged and it's in the release.

[hypest]

Since we're still on the path to implement the feature in the RN side, let's close this one. We can reopen/resume if we change course.

@etoledom , even if merged, is the iOS side of this in actual use or needed for the RN based implementation? If not, let's revert if possible. WDYT?

[etoledom]

I don't think that it is in use, and @koke reverted those changes on wordpress-mobile/gutenberg-mobile#275 . So we should be good to go 👍

[marecar3]

Since we're still on the path to implement the feature in the RN side, let's close this one. We can reopen/resume if we change course.

Agree.