Polish the button #3427

jasmussen · 2017-11-10T14:01:39Z

This fixes #3283.

This is a redesign of the button block to be more neutral looking. In addition to this, it makes the URL aspect inline, so it behaves just like how a caption does on an image.

GIF:

This fixes #3283. This is a redesign of the button block to be more neutral looking. In addition to this, it makes the URL aspect inline, so it behaves just like how a caption does on an image.

youknowriad · 2017-11-10T15:16:09Z

blocks/library/button/index.js

@@ -130,6 +119,18 @@ registerBlockType( 'core/button', {
 					</InspectorControls>
 				}
 			</span>,
+			focus && (
+				<form
+					className="blocks-format-toolbar__inline-link-modal"


I think we do not want to share the style of this class with other blocks, it may make sense to rename the classname to match our guidelines. Something like blocks-button__inline-link. Ans style it as part of the editor.scss of the button block

youknowriad · 2017-11-10T15:18:05Z

Nice job, I like this new design way better. 👍

I noticed some small design bugs

The submit button is crossing the border when the button is left/right aligned

Should we align the link form on the left when the button is not centered?

jasmussen · 2017-11-10T15:23:33Z

Thanks for the review, good stuff all around. I'll fix it next week. Any idea why Travis is complaining btw? Also, have a nice weekend!

youknowriad · 2017-11-10T15:27:39Z

blocks/library/button/index.js

+					/>
+					<IconButton icon="editor-break" label={ __( 'Apply' ) } type="submit" />
+				</form>
+			)


@jasmussen Yep, travis is failing because of a missing trailing comma after this parenthesis. You should definitely install an eslint plugin to your favorite IDE ;)

💓💓👍🌌

Have a nice weekend Joen :)

karmatosed · 2017-11-10T19:38:32Z

I will hold off my design review until you have done those fixes @jasmussen - just let me know when want one.

- Move to button-specific class - Align url to match button alignment - Fix input field overlap

codecov · 2017-11-13T08:49:48Z

Codecov Report

Merging #3427 into master will increase coverage by 0.89%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3427      +/-   ##
==========================================
+ Coverage    33.8%   34.69%   +0.89%     
==========================================
  Files         249      250       +1     
  Lines        6713     6842     +129     
  Branches     1207     1262      +55     
==========================================
+ Hits         2269     2374     +105     
- Misses       3754     3773      +19     
- Partials      690      695       +5

Impacted Files	Coverage Δ
blocks/library/button/index.js	`20% <0%> (ø)`	⬆️
editor/store-defaults.js	`100% <0%> (ø)`	⬆️
editor/header/index.js	`0% <0%> (ø)`	⬆️
blocks/library/image/block.js	`0% <0%> (ø)`	⬆️
utils/viewport.js	`100% <0%> (ø)`
components/autocomplete/index.js	`78.82% <0%> (+1.35%)`	⬆️
editor/block-mover/mover-label.js	`100% <0%> (+4.54%)`	⬆️
editor/block-mover/index.js	`4.76% <0%> (+4.76%)`	⬆️

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 1fb6853...34f3f4e. Read the comment docs.

jasmussen · 2017-11-13T08:51:04Z

Thanks again, ready for final review now. I've addressed the feedback and fixed the linting issue. Can you recommend a linter for Atom? The last one I used sort of broke.

Screenshots:

youknowriad · 2017-11-13T09:17:12Z

Based on quick search, this looks like the widely used eslint atom plugin https://atom.io/packages/linter-eslint (I don't use atom myself though)

youknowriad

LGTM 👍

hedgefield · 2017-11-13T10:39:46Z

Good stuff! One question and one remark:

What does the Stand on a line option in the block settings mean?
The Apply button should have some sort of "this has been successfully applied" feedback. When I entered a URL and pressed Enter, nothing seemed to change, so I clicked the button, and still nothing visibly changed. It would be good to hide (or at least dim) the Apply button once you've successfully entered something, and only show it again when you change something in the URL.

jasmussen · 2017-11-13T10:50:49Z

What does the Stand on a line option in the block settings mean?

Perhaps it would be nice to be able to show little ? help icons, with elaborating tooltips. In this case, "Stand on a line" is basically the same as applying overflow: hidden; to the block container. It prevents successive floated blocks from stacking up to the right of a left floated button. It's a "float clearer", in other words. Perhaps we should open a separate ticket for little help icons with tooltips?

The Apply button should have some sort of "this has been successfully applied" feedback.

I don't disagree. Can you think of other areas where an "applied" affordance would be helpful? The pattern emulated with this link was the caption on images. If we can think of a few places, it might be good to ticket separately and then reference all the places where such an affordance would help.

hedgefield · 2017-11-13T11:04:00Z

Ahhh, that's something I would call "full width" :) so coming up with a good general term for that would be useful. It's an issue with images too, though not so much the language there, but the concept, the difference between full width and center aligned: #3325. If we can come up with a good term we can use the 'stand on a line' toggle to solve the problem there too. 👍

Making a separate issue for the Apply button is a good idea. Off the top of my head, it will be used in links, captions, buttons, permalink.. cover image has a link field too, maybe the shortcode block.

karmatosed · 2017-11-13T21:12:20Z

Perhaps we should open a separate ticket for little help icons with tooltips?

Just a little note, I am cautious of us adding help icons. I am totally into us being clearer, but if we have to add a help icon maybe we aren't being in first place? That said, help has a place, it should always though mean we look first at if we can not add it.

I do think 'Stand on a line' could do with some work. Maybe literally say what it does?

"Ensure clears to new line" or something a little less clumsy.

As for an applied notice, I agree, it's probably a useful feedback mechanism. I started a ticket to consider the possible areas this could be. #3461

Polish the button

5288ac2

This fixes #3283. This is a redesign of the button block to be more neutral looking. In addition to this, it makes the URL aspect inline, so it behaves just like how a caption does on an image.

jasmussen added the Design label Nov 10, 2017

jasmussen self-assigned this Nov 10, 2017

jasmussen requested review from karmatosed and youknowriad November 10, 2017 14:01

youknowriad reviewed Nov 10, 2017

View reviewed changes

Address feedback

0dd0a55

- Move to button-specific class - Align url to match button alignment - Fix input field overlap

Fix linting issue

34f3f4e

youknowriad approved these changes Nov 13, 2017

View reviewed changes

jasmussen merged commit 67a3a63 into master Nov 13, 2017

Soean mentioned this pull request Nov 13, 2017

Button: URL input field is hidden on small screens #2635

Closed

2 tasks

youknowriad deleted the polish/button branch November 13, 2017 10:37

hedgefield mentioned this pull request Nov 14, 2017

Add in applied feedback messages #3461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish the button #3427

Polish the button #3427

jasmussen commented Nov 10, 2017

youknowriad Nov 10, 2017

youknowriad commented Nov 10, 2017

jasmussen commented Nov 10, 2017

youknowriad Nov 10, 2017 •

edited

Loading

jasmussen Nov 10, 2017

youknowriad Nov 10, 2017

karmatosed commented Nov 10, 2017

codecov bot commented Nov 13, 2017 •

edited

Loading

jasmussen commented Nov 13, 2017

youknowriad commented Nov 13, 2017

youknowriad left a comment

hedgefield commented Nov 13, 2017

jasmussen commented Nov 13, 2017

hedgefield commented Nov 13, 2017 •

edited

Loading

karmatosed commented Nov 13, 2017

Polish the button #3427

Polish the button #3427

Conversation

jasmussen commented Nov 10, 2017

youknowriad Nov 10, 2017

Choose a reason for hiding this comment

youknowriad commented Nov 10, 2017

jasmussen commented Nov 10, 2017

youknowriad Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

jasmussen Nov 10, 2017

Choose a reason for hiding this comment

youknowriad Nov 10, 2017

Choose a reason for hiding this comment

karmatosed commented Nov 10, 2017

codecov bot commented Nov 13, 2017 • edited Loading

Codecov Report

jasmussen commented Nov 13, 2017

youknowriad commented Nov 13, 2017

youknowriad left a comment

Choose a reason for hiding this comment

hedgefield commented Nov 13, 2017

jasmussen commented Nov 13, 2017

hedgefield commented Nov 13, 2017 • edited Loading

karmatosed commented Nov 13, 2017

youknowriad Nov 10, 2017 •

edited

Loading

codecov bot commented Nov 13, 2017 •

edited

Loading

hedgefield commented Nov 13, 2017 •

edited

Loading