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.

Sign up for GitHub

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

Jump to bottom

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

@noisysocks
Revert "Remove "Open in new window" link option (#4583)"
eb2454d 
This reverts commit 5ab37b7.
@noisysocks
Fix 'Open in new window' link option
f46cd44 
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
@noisysocks
Use ellipsis icon for opening link settings
b12be5b 
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.
@noisysocks
Use link toolbar button to remove links
5f916d7 
Consolidate the dedicated 'remove link' button into the toolbar button
that adds a link.
@noisysocks
After editing a link, exit editing mode
4a9ff7a 
Reset the link UI once the user has edited the link value.
@noisysocks
Fix error when setting 'Open in new window' on a new link
d111047 
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).
@noisysocks
Expand link UI to fit contents
8e64f33 
The link popup should widen to fit the URL within it.
@noisysocks
Improve suggested links UI in link inserter
94ab326 
Make the suggested links UI more stylistically consistent with the rest
of the editor.
@noisysocks
Move settings button to right of the link UI
0bb6c85 
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
karmatosed
karmatosed approved these changes Apr 9, 2018
View reviewed changes
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.

@noisysocks noisysocks mentioned this pull request Apr 11, 2018
Merged
@noisysocks noisysocks added this to the 2.7 milestone Apr 13, 2018
This was referenced Sep 10, 2018
Closed
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karmatosed karmatosed karmatosed approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@aduth aduth Awaiting requested review from aduth

Assignees
No one assigned
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
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@noisysocks @ellatrix @karmatosed