FIX - Improve Link Entry #2779

tg-ephox · 2017-09-25T04:41:05Z

Description

Link entry for Paragraph, Cover Image and Button blocks have all been made similar to the Image block (with the addition of animations on entry/exit)

Formatting toolbars for Cover Image and Image have been modified so that they appear in the single toolbar above the block, so that this is consistent between all blocks, previously these were the only toolbars that were centred.

fixes #2715

How Has This Been Tested?

Unit tests, manual browser testing.

Screenshots (jpeg or gifs if applicable):

Types of changes

New feature

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

codecov · 2017-09-25T04:41:09Z

Codecov Report

Merging #2779 into master will increase coverage by 3.68%.
The diff coverage is 75.64%.

@@            Coverage Diff             @@
##           master    #2779      +/-   ##
==========================================
+ Coverage   34.07%   37.76%   +3.68%     
==========================================
  Files         192      199       +7     
  Lines        5675     6379     +704     
  Branches      996     1190     +194     
==========================================
+ Hits         1934     2409     +475     
- Misses       3165     3330     +165     
- Partials      576      640      +64

Impacted Files	Coverage Δ
blocks/library/cover-image/index.js	`33.33% <ø> (ø)`	⬆️
blocks/library/embed/index.js	`45.55% <ø> (ø)`	⬆️
blocks/library/video/index.js	`21.05% <ø> (ø)`	⬆️
blocks/library/button/index.js	`21.05% <0%> (+1.05%)`	⬆️
blocks/editable/format-toolbar/index.js	`12% <0%> (+5.61%)`	⬆️
blocks/library/image/block.js	`0% <0%> (ø)`	⬆️
blocks/url-input/index.js	`18.03% <100%> (+16.39%)`	⬆️
components/higher-order/with-focus-return/index.js	`92.3% <100%> (+0.64%)`	⬆️
blocks/url-input/button.js	`75% <62.5%> (+75%)`	⬆️
blocks/url-input/ancestor-size.js	`75% <75%> (ø)`
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27cd1f5...218de2f. Read the comment docs.

youknowriad · 2017-09-25T14:11:35Z

blocks/editable/format-toolbar/index.js

@@ -40,22 +38,9 @@ const DEFAULT_CONTROLS = [ 'bold', 'italic', 'strikethrough', 'link' ];
 class FormatToolbar extends Component {


I guess we could refactor this component to be just a functional component?

I don't think we can as it needs the lifecycle methods?

youknowriad · 2017-09-25T14:16:59Z

I'm seeing a light "white" layer on the border of the link popover submit button

youknowriad · 2017-09-25T14:19:23Z

I like the fact that we have a unique way of adding links to the components, though, I'm finding that this requries a lot of steps to add the link.

Add a link
Tweak the options
Validate

Maybe the options could be triggered by a gear button or something. anyway, a more "designy" feedback would be great. @jasmussen @karmatosed

youknowriad · 2017-09-25T14:19:54Z

The button link is not being saved as expected (check the text mode), Not sure if it's related to this PR.

youknowriad · 2017-09-25T14:30:32Z

the appearing animation is a bit slow IMO, could we increase its speed?

youknowriad · 2017-09-25T14:30:50Z

I noticed that the link popover is not showing up as expected for the inline toolbar (image captions for example)

tg-ephox · 2017-09-26T06:22:12Z

This isn't the correct text for a Button block?

<!-- wp:core/button {"align":"center"} -->
<div class="wp-block-button aligncenter"><a href="https://github.com/WordPress/gutenberg"><span>Help build Gutenberg</span></a></div>
<!-- /wp:core/button -->

youknowriad · 2017-09-26T08:21:04Z

@tg-ephox for some reason, the a tag was empty (no href) for me. I'll dig more later.

tg-ephox · 2017-09-27T03:40:43Z

@youknowriad the dialog that pops in allows you to 'un-link', which will result in a anchor tag with no href, maybe this option shouldn't be available for the Button block?

youknowriad · 2017-09-27T07:50:18Z

Maybe i'm doing something wrong but here's the screencast

tg-ephox · 2017-09-29T01:12:49Z

@youknowriad I found the URL problem for Button, I'd renamed a variable...

tg-ephox · 2017-10-05T05:26:25Z

hey @annaephox @karmatosed can I pls get some design feedback on this?

anna-harrison · 2017-10-06T04:18:56Z

Hey @tg-ephox : looks really good to me!
@karmatosed : our tests from this feature in TinyMCE mobile showed that people find the serialised link dialog intuitive to use. We have tested the Gutenberg version informally at this stage, and results confirm the same
@youknowriad : are we good to merge?

youknowriad · 2017-10-06T08:55:12Z

Great job so far! I'm really liking the consistency.

Confirmed the button block is working now
The inline toolbar is placed at the same level of the block toolbar, I'm not sure this was part of the initial plan of this PR, We'll need a design validation for this @mtias @jasmussen @karmatosed
Minor: Could we make sure to submit this when we hit "Enter"?

youknowriad · 2017-10-06T08:58:19Z

blocks/library/button/index.js

@@ -75,19 +75,9 @@ registerBlockType( 'core/button', {
 					onChange={ ( value ) => setAttributes( { text: value } ) }
 					formattingControls={ [ 'bold', 'italic', 'strikethrough' ] }
 					keepPlaceholderOnFocus
+					extraToolbarButtons={ <UrlInputButton showSettings={ false } url={ url } onChange={ updateUrl } /> }


Why do we need this prop? Can't we just put this putting in the BlockControls component?

youknowriad · 2017-10-06T08:59:46Z

blocks/url-input/button.js

+const EditStep = 'EDIT';
+const SettingsStep = 'SETTINGS';
+
+const AllSteps = [ EditStep, SettingsStep, DisplayStep ];


We have this unofficial style guideline for uppercasing module constants? Could we uppercase those?

const DISPLAY_STEP...

youknowriad · 2017-10-06T09:02:00Z

blocks/url-input/button.js

+		if ( steps[ currentStep ] === EditStep ) {
+			return [
+				<UrlInput key="urlinput" value={ url || '' } onChange={ this.changeLink } required={ ! isDeleted } />,
+				<IconButton key="iconbutton" className="blocks-url-input__unlink" icon="dismiss" label={ __( 'Un-link' ) } disabled={ ! url } onClick={ this.deleteLink } />,


Could we break this line and the above maybe to improve readability?

youknowriad · 2017-10-06T09:02:32Z

blocks/url-input/button.js

+	renderStep( steps ) {
+		const { currentStep, url, isDeleted, opensInNewWindow } = this.state;
+
+		if ( steps[ currentStep ] === EditStep ) {


Why not a switch?

karmatosed · 2017-10-11T17:13:25Z

@annaephox I am not happy merging it in it's current state. The design discussion is here: #2715

jasmussen · 2017-10-12T08:09:42Z

Thanks for working on this, sorry for coming in with feedback so late. I'm also coming in fresh, so please let me know if some of the following are known issues, or separate from this PR.

The external link toggle can't be set and immediately unset:

Just click twice on the switch.

The link dialog will sometimes move if I click to edit a link, then click around in the dialog a bit:

The caret is moved to the start of the input field when I click around. Pretty sure this is the same bug as 2 mentioned above, but perhaps it can help narrow down what happens:

The block level link (i.e. for images) looks a bit off. This is what Riad refers to in this comment:

The inline toolbar is placed at the same level of the block toolbar, I'm not sure this was part of the initial plan of this PR

Two solutions I can see. The easiest one is probably to let the link dialog box sit stacked below the quick toolbar.

The other solution is to keep it where it is — i.e cover the quick toolbar. But then we can't have stacked shadows, i.e. the quick toolbar should probably be invisible when the link toolbar is there, and reappear when done. Also, we should make sure they have the same height — right now the link toolbar is slightly taller:

Given the discussion ongoing in #2983, perhaps it's best to do as little as possible around this area right now.

mikekosulin · 2017-11-19T22:34:40Z

How to setup option:
Open a new window by default?

karmatosed · 2017-12-14T21:14:54Z

I am closing this as we can always iterate but we reached a point here where we weren't progressing. Let's get feedback in testing and take from there.

Make inline link entry same as image block

f3a8d8f

tg-ephox added 2 commits September 25, 2017 15:39

Fix wrapping on long url, dont show settings for image link

f69e1c8

Adding tests

e81c4f0

youknowriad reviewed Sep 25, 2017

View reviewed changes

youknowriad added the Needs Design Feedback Needs general design feedback. label Sep 25, 2017

tg-ephox added 3 commits September 26, 2017 11:52

Finished tests

c714b95

Fix button border z-index

ad724e9

modify image block toolbars, increase speed of link entry animation

ccda5e3

anna-harrison mentioned this pull request Sep 26, 2017

Improve Link Entry #2715

Closed

tg-ephox self-assigned this Sep 27, 2017

Fixed link button URL not being added

00b357c

Fix linting

8d00a6f

youknowriad reviewed Oct 6, 2017

View reviewed changes

tg-ephox added 2 commits October 9, 2017 11:12

Merge branch 'master' into try/2715-improve-link-entry

f29ed5e

ImageBlock - differentiate focus between image and caption

1ff418f

tg-ephox added 2 commits October 9, 2017 14:53

Make caption toolbar consistent on video and embed blocks

75bc17f

Address review issues

3d886b2

Resize link entry based on toolbar group size

218de2f

jasmussen mentioned this pull request Oct 13, 2017

Text Block: Sometimes the cursor gets moved behind the first character #1607

Closed

karmatosed closed this Dec 14, 2017

youknowriad deleted the try/2715-improve-link-entry branch December 15, 2017 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX - Improve Link Entry #2779

FIX - Improve Link Entry #2779

tg-ephox commented Sep 25, 2017 •

edited

Loading

codecov bot commented Sep 25, 2017 •

edited

Loading

youknowriad Sep 25, 2017

tg-ephox Sep 26, 2017

youknowriad commented Sep 25, 2017 •

edited

Loading

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

tg-ephox commented Sep 26, 2017

youknowriad commented Sep 26, 2017

tg-ephox commented Sep 27, 2017

youknowriad commented Sep 27, 2017

tg-ephox commented Sep 29, 2017 •

edited

Loading

tg-ephox commented Oct 5, 2017

anna-harrison commented Oct 6, 2017 •

edited

Loading

youknowriad commented Oct 6, 2017

youknowriad Oct 6, 2017

youknowriad Oct 6, 2017

youknowriad Oct 6, 2017

youknowriad Oct 6, 2017

karmatosed commented Oct 11, 2017

jasmussen commented Oct 12, 2017

mikekosulin commented Nov 19, 2017

karmatosed commented Dec 14, 2017

		@@ -40,22 +38,9 @@ const DEFAULT_CONTROLS = [ 'bold', 'italic', 'strikethrough', 'link' ];
		class FormatToolbar extends Component {

FIX - Improve Link Entry #2779

FIX - Improve Link Entry #2779

Conversation

tg-ephox commented Sep 25, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

codecov bot commented Sep 25, 2017 • edited Loading

Codecov Report

youknowriad Sep 25, 2017

Choose a reason for hiding this comment

tg-ephox Sep 26, 2017

Choose a reason for hiding this comment

youknowriad commented Sep 25, 2017 • edited Loading

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

youknowriad commented Sep 25, 2017

tg-ephox commented Sep 26, 2017

youknowriad commented Sep 26, 2017

tg-ephox commented Sep 27, 2017

youknowriad commented Sep 27, 2017

tg-ephox commented Sep 29, 2017 • edited Loading

tg-ephox commented Oct 5, 2017

anna-harrison commented Oct 6, 2017 • edited Loading

youknowriad commented Oct 6, 2017

youknowriad Oct 6, 2017

Choose a reason for hiding this comment

youknowriad Oct 6, 2017

Choose a reason for hiding this comment

youknowriad Oct 6, 2017

Choose a reason for hiding this comment

youknowriad Oct 6, 2017

Choose a reason for hiding this comment

karmatosed commented Oct 11, 2017

jasmussen commented Oct 12, 2017

mikekosulin commented Nov 19, 2017

karmatosed commented Dec 14, 2017

tg-ephox commented Sep 25, 2017 •

edited

Loading

codecov bot commented Sep 25, 2017 •

edited

Loading

youknowriad commented Sep 25, 2017 •

edited

Loading

tg-ephox commented Sep 29, 2017 •

edited

Loading

anna-harrison commented Oct 6, 2017 •

edited

Loading