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

pre-fill link input with the current href value #59

Closed
wants to merge 2 commits into from

Conversation

ZeroCho
Copy link
Contributor

@ZeroCho ZeroCho commented Jun 14, 2016

#15 I fixed this and slightly changed the way the link button acts.

Currently, Popover IconButton of Link is enabled when both hasSelection and isCursorOnLink,
but I think that it should be enabled only when hasSelection because you cannot change href value when isCursorOnLink. I think users should be notified that link can be set only when they select block, not when they put cursor on link.

By the way, I'm not sure my codes fit your coding style.

@sstur
Copy link
Owner

sstur commented Jun 15, 2016

Thanks for your contribution! I like the fix, but I prefer a different behavior for editing links when the cursor is on it.

Currently, Popover IconButton of Link is enabled when both hasSelection and isCursorOnLink

That's not quite correct. The current behavior is that the button is enabled when either hasSelection or isCursorOnLink.

I think that it should be enabled only when hasSelection because you cannot change href value when isCursorOnLink

You can indeed change the href value when isCursorOnLink. Actually I think that's the preferred behavior. I'd like it so that when you submit the InputPopover when the cursor is on a link the link gets updated with the new value (and selected afterwards to communicate with the user what was changed).

That was my intent with this.

Here's where it gets tricky though: if the editor contains "hello world click me" and the user selects the words "world click" and attempts to add a link, then:

  1. Should we pre-populate the InputPopover? (what if parts of two separate links are selected?)
  2. If they submit a new (or even unchanged) url, should we unlink only the selected text and then wrap it in a new link?

I'm thinking we should (1) not pre-populate it and (2) yes, clear the text of any links and create a new link.

Thoughts?

@ZeroCho
Copy link
Contributor Author

ZeroCho commented Jun 15, 2016

Currently, Popover IconButton of Link is enabled when both hasSelection and isCursorOnLink
haha.

You're right. It is Either not Both. It was my mistake.

You can indeed change the href value when isCursorOnLink.

No, I detected that I couldn't change the value. I'll show you the example
ex1
I typed ZeroCho and linked it with zerocho.com

ex2
I put cursor on link "ZeroCho" (between o and C exactly)

ex3
and I changed the value to google.com

ex4
but still the same value, zerocho.com. I tested with chrome browser and your react-rte.org/demo

The main reason I wanna change the default behavior is...
ex5

Like the above case, when people put cursor between two links, editor doesn't know which link to edit. Former or latter? So, that's why I thought that isCursorOnLink is not appropriate.

And I'll try to fix (1), (2)! Also, please answer #16

Here's where it gets tricky though: if the editor contains "hello world click me" and the user selects the words "world click" and attempts to add a link, then:

What do you think this editor should behave if a user selects only 'cli' which is the part of link, and clicks the link button?
Should the link form pre-fills the href value of 'click me'?
Or, should it pre-fills nothing because there is a possibility that the user wanna connect a new link to 'cli' instead of 'click me'?

@sstur
Copy link
Owner

sstur commented Jul 2, 2016

I think I know why your link is not updating in the demo: because entities are kept outside of ContentState so any change to an entity does not cause a change to ContentState and no re-render will happen. This is a known issue with DraftJS.

I'd create a new entity and then update the ContentState accordingly. That should do it.

About the behavior of selecting and linking text, I have a plan. I'll try to detail it in #15.

@sstur sstur mentioned this pull request Jul 2, 2016
@sstur sstur closed this Aug 21, 2016
@gpittarelli gpittarelli mentioned this pull request Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants