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

Iterate on 'Insert link' interface #6028

Merged
merged 9 commits into from
Apr 10, 2018
Merged

Iterate on 'Insert link' interface #6028

merged 9 commits into from
Apr 10, 2018

Conversation

noisysocks
Copy link
Member

Closes #4572.

linky-links

Testing

  1. Create a paragraph block.
  2. Highlight some text and click the Link button in the formatting toolbar.
  3. Create a link by searching for a page.
  4. Create a link by typing in an URL.
  5. Toggle on/off Open in new window and check that the generated markup receives a target="_blank".
  6. Select an existing link and edit it by clicking on the Edit button in the popup.
  7. Select an existing link and remove it by clicking on the Unlink button in the formatting toolbar.

@noisysocks noisysocks added [Type] Enhancement A suggestion for improvement. [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Needs Design Feedback Needs general design feedback. labels Apr 6, 2018
@ellatrix
Copy link
Member

ellatrix commented Apr 6, 2018

Restores the Open in new window setting by reverting #4583.

😭 but ok. Wouldn't the ellipsis be better placed on the other side? Similar to blocks, and doesn't deserve first focus imho.

@karmatosed
Copy link
Member

Wouldn't the ellipsis be better placed on the other side? Similar to blocks, and doesn't deserve first focus imho.

I do like it before but not oppose to having secondary after edit, it does feel more the same as our current patterns so lets do that change @noisysocks.

Can the link icon be only at full opacity once then text is selected? This could fix some issues with people just having no text selected see: #5960

Some changes had occured elsewhere in the codebase in between when this
UI was reverted and when it was unreverted.

- Imports `ToggleControl` correctly
- Fixes `RichText` tests
Change the icon to be the same icon used elsewhere in the editor, and
move the toggle button to the left of the link UI.
Consolidate the dedicated 'remove link' button into the toolbar button
that adds a link.
Reset the link UI once the user has edited the link value.
Don't call `onChange()` when 'Open in new window' is toggled if we are
adding a new link (as opposed to editing an existing link).
The link popup should widen to fit the URL within it.
Make the suggested links UI more stylistically consistent with the rest
of the editor.
Move the ellipsis button to the right of the edit button. This is more
consistent with where the ellipsis icon is used elsewhere in the editor.
@noisysocks
Copy link
Member Author

I do like it before but not oppose to having secondary after edit, it does feel more the same as our current patterns so lets do that change @noisysocks.

👌 done in 0bb6c85.

Can the link icon be only at full opacity once then text is selected? This could fix some issues with people just having no text selected see: #5960

Let's treat this as a seperate issue since #5960 is open and I have some questions about it (namely #5960 (comment)).

@noisysocks noisysocks removed the Needs Design Feedback Needs general design feedback. label Apr 9, 2018
@noisysocks noisysocks requested a review from aduth April 9, 2018 05:21
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Looks great to me, lets get this in and take from there any iterations, I agree.

@noisysocks noisysocks merged commit 43a58b2 into master Apr 10, 2018
@noisysocks noisysocks deleted the update/link-interface branch April 10, 2018 02:19
@karmatosed
Copy link
Member

@noisysocks we seem to have a fun bug :) Dancing link interface:

2018-04-10 18 02 19

Happens for me in Chrome and Firefox. Props to @mtias for finding this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants