Cover Image Block: Added button. by jorgefilipecosta · Pull Request #3047 · WordPress/gutenberg

jorgefilipecosta · 2017-10-18T09:18:27Z

Description

This PR adds an optional button to the cover image. Allowing button customization options. As specified in issue #2765.

Testing

Add a cover image. Verify everything works as before.
Add a button verify things worked as before.
In the cover image enable show button
Customize the button of the cover image see things worked as expected.
Save the post see the result, see if the result is ok.
Open a post with a cover image and button created before this changes and see if they load as expected.

Screenshots:

Notes:

In order to not repeat button block code, 2 components were extracted one that renders button editor and other that render button for saving. This 2 components can be used by other blocks that need a button.
The CSS was a little by trial error maybe it can be improved, but in my tries, this looked like the simplest way.

mcsf

Nice work!

I identified a few things, though no big blocker.

The CSS was a little by trial error maybe it can be improved, but in my tries, this looked like the simplest way.

Worth mentioning that, while this looks great on the Gutenberg Theme, it doesn't always on Twenty Fifteen:

Finally, this PR makes a previous issue more prominent: the placeholder text for Cover Image isn't always positioned to match the title field, potentially resulting in a user's inability to focus the field. See screencast. Let's revisit that separately.

Assuming the little things are addressed, 🚢 !

mcsf · 2017-10-18T12:00:06Z

blocks/library/button/index.js

+				setButtonBackgroundColor={ ( value ) => setAttributes( { color: value } ) }
+				setButtonText={ ( value ) => setAttributes( { text: value } ) }
+				setButtonTextColor={ ( value ) => setAttributes( { textColor: value } ) }
+				setButtonUrl={ ( value ) => setAttributes( { url: value } ) }


focus is already passed as a prop, so we have duplication. Since the name “focus” is already customary, should we just get rid of showInspector?

mcsf · 2017-10-18T12:12:10Z

blocks/library/button/button-controls.js

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


As mentioned in the other file, focus && is the typical form anyway :).

mcsf · 2017-10-18T12:47:33Z

blocks/library/button/index.js

+				{ ...{ className, focus, setAttributes, url, setFocus, title } }
+				buttonBackgroundColor={ color }
+				buttonText={ text }
+				buttonTextColor={ textColor }


Similarly, buttonUrl is a duplicate of url. Pick your favorite. :)

mcsf · 2017-10-18T13:06:46Z

blocks/library/button/index.js

-					</InspectorControls>
-				}
-			</span>,
+			<ButtonControls key="buttonControls"


It doesn't look like setAttributes is needed in ButtonControls, since setters are now provided for every datum (setButtonText, etc.).

mcsf · 2017-10-18T13:12:29Z

blocks/library/cover-image/index.js

 				'has-parallax': hasParallax,
 			}
 		);
+		const focusedEditable = focus ? focus.editable || 'title' : null;


This is my personal take on mixing infix operators, but I don't trust that we all know the full operator precedence table to make sure we don't write something we don't mean. :)

In other words, this may be unambiguous for the interpreter, but it's ambiguous for us. I suggest focus ? ( focus.editable || 'title' ) : null.

mcsf · 2017-10-18T13:16:36Z

blocks/library/cover-image/index.js

+		buttonUrl: {
+			type: 'string',
+		},
+		showButton: {


Minor: I would prefer hasButton (or even shouldShowButton, though lengthier), since in other contexts showButton could be read as an action.

mcsf · 2017-10-18T13:52:56Z

blocks/library/button/button-render.js

@@ -0,0 +1,16 @@
+export default ( { align,


Missing line break before align

mtias · 2017-10-18T14:53:41Z

I don't think we should be importing an internal like /button/button-render from another block. At most, it should be a reusable piece in /blocks/{colorable-button} that other blocks could leverage. But still, I don't think it's a great strategy going forwards.

The right solution is going to be nested blocks and specifying which blocks are included by the default. In this case, since it's an interim solution, I would still duplicate the code for the button and keep it as an internal of cover image.

codecov · 2017-10-19T13:57:58Z

Codecov Report

Merging #3047 into master will decrease coverage by 0.04%.
The diff coverage is 8.33%.

@@            Coverage Diff             @@
##           master    #3047      +/-   ##
==========================================
- Coverage   31.36%   31.31%   -0.05%     
==========================================
  Files         223      223              
  Lines        6393     6403      +10     
  Branches     1136     1139       +3     
==========================================
  Hits         2005     2005              
- Misses       3685     3692       +7     
- Partials      703      706       +3

Impacted Files	Coverage Δ
blocks/library/cover-image/index.js	`23.52% <8.33%> (-9.81%)`	⬇️

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 c5ebeca...4d227e8. Read the comment docs.

jorgefilipecosta · 2017-10-19T14:41:19Z

Hi @mcsf, @mtias, thank you for your feedback 👍 I think I addressed all fo it, including the case where editable not matched placeholder position (nice catch). Regarding the problem, in some themes, a margin is set for h2 and I made a rule that sets the margin to 0 inside our block which is essential for things to look nice.

aduth · 2017-10-19T20:53:33Z

blocks/library/cover-image/editor.scss

+
+	.button-container {
+		.blocks-format-toolbar__link-modal {
+			z-index: 2;


To help maintain sensible z-index, there is a mapping entry and corresponding function that any z-index property should be using:

https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_z-index.scss

aduth · 2017-10-19T20:54:45Z

blocks/library/cover-image/editor.scss

 	}
+
+	.button-container {
+		.blocks-format-toolbar__link-modal {


Do we need to nest this so much? Part of the benefit of our BEM-like pattern for CSS naming is that we're guaranteed a reasonable amount of uniqueness, so unless specificity or context matters, it's usually safe to put at the top level.

aduth · 2017-10-19T20:55:41Z

blocks/library/cover-image/index.js

+							</form>
+						}
+						{ focus &&
+							<InspectorControls key="inspector">


Do we need key?

aduth · 2017-10-19T20:55:57Z

blocks/library/cover-image/index.js

 				) : null }
+
+				{ hasButton &&
+					<span key="button" className="button-container aligncenter" style={ { backgroundColor: buttonBackgroundColor } } >


Do we need key?

aduth · 2017-10-19T20:56:20Z

blocks/library/cover-image/index.js

+					</div>
 				) : null }
+
+				{ hasButton &&


Is it possible for us to be able to infer hasButton based on presence of other button related attributes, rather than creating a separate one specifically for it? Maybe when toggling "Show Button", we assign buttonText as an empty string, then detect hasButton as buttonText !== undefined (or typeof buttonText === 'string')?

aduth · 2017-10-19T21:04:54Z

blocks/library/cover-image/style.scss

 	justify-content: center;
 	align-items: center;
-
+	


Not explicitly documented for CSS guidelines, but in general we should remove trailing whitespace.

PHP reference: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#remove-trailing-spaces

I also suggest downloading an EditorConfig plugin for your editor, which should simplify this task for you since we include a configuration file:

http://editorconfig.org/#download

aduth · 2017-10-19T21:05:41Z

blocks/library/cover-image/style.scss

 	}
+
+	.button-container {
+		top: 100px;


It might be simpler to use margins here instead of relative positioning.

aduth · 2017-10-19T21:06:16Z

blocks/library/cover-image/style.scss

+
+	.button-container {
+		top: 100px;
+		font-family: $default-font;


Why do we explicitly override the font family?

Hi @aduth, this logic was already being applied in button code. I did some checks and without it, in some themes, we use other fonts that don't look as nice in the button. So, for now, I kept this font family override.

aduth · 2017-10-19T21:06:56Z

blocks/library/cover-image/style.scss

+}

+.wp-block-cover-image > h2 {
+	margin: 0px !important;


Do we need !important? Is this an issue of specificity? We should avoid !important as much as possible.

aduth · 2017-10-19T21:08:34Z

blocks/library/cover-image/index.js

+						{ focusedEditable === 'button' &&
+							<form
+								className="blocks-format-toolbar__link-modal"
+								onSubmit={ ( event ) => event.preventDefault() }>


Why do we render this as a form if we're not doing anything form-like with it? Seems we could just as easily drop the form and avoid assigning type="submit" to the icon button.

aduth · 2017-10-19T21:09:53Z

Also, this has me desperately wishing for proper block nesting, as the many additional attributes necessary to support a button in this block is less than ideal.

jorgefilipecosta · 2017-10-23T15:04:22Z

Hi @aduth thank you a lot for your feedback and tips! It was applied. I agree that nesting would be a better solution, as soon as we have nesting available, I can refactor this logic and show button toggle UI to automatically add a button inside. The downside is that the change may make the blocks of this version incompatible.

mtias · 2017-10-24T16:06:51Z

Placeholder text can get stuck.

mtias · 2017-10-24T16:07:39Z

What about calling the toggle "call to action"? (cc @jasmussen @karmatosed for thoughts.)

ellatrix · 2017-10-24T16:07:54Z

@matias See #2865.

karmatosed · 2017-10-24T17:18:18Z

Call to action works for me @mtias.

jorgefilipecosta · 2017-10-25T21:49:42Z

Hi @mtias, @karmatosed the toggle text was updated to "Call to action" as suggested :)

Dismissing my "Needs Changes" review, but I'm personally of the opinion that we put more effort toward block nesting and avoid this interim solution altogether.

jorgefilipecosta · 2017-10-31T09:04:47Z

Thank you for your reviews @aduth 👍 I understand that nesting would be a better approach if we prefer to wait I can revisit this and implement it when nesting is ready.

jorgefilipecosta · 2017-11-01T09:39:53Z

Hi @mtias, what is your opinion should we merge this PR that adds a button to cover image as a setting and make it available on next release or it is better to wait for nesting and rethink this changes when nesting is ready?

jorgefilipecosta added [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. [Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement. labels Oct 18, 2017

jorgefilipecosta self-assigned this Oct 18, 2017

jorgefilipecosta mentioned this pull request Oct 18, 2017

Add button to cover image #2765

Closed

jorgefilipecosta requested review from aduth, ellatrix, gziolo and youknowriad October 18, 2017 09:22

jorgefilipecosta force-pushed the add/button-cover-image branch from f9bfdc5 to 20b2fb6 Compare October 18, 2017 09:29

jorgefilipecosta requested a review from mcsf October 18, 2017 09:31

mcsf approved these changes Oct 18, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/button-cover-image branch 3 times, most recently from 8fc557d to 8459b01 Compare October 19, 2017 13:57

aduth previously requested changes Oct 19, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/button-cover-image branch 3 times, most recently from 3fb89ee to ed6c5f9 Compare October 23, 2017 14:25

jorgefilipecosta added 2 commits October 30, 2017 08:52

Cover Image Block: Added button.

be855cf

Update button toggle to call to action.

4d227e8

jorgefilipecosta force-pushed the add/button-cover-image branch from 620595d to 4d227e8 Compare October 30, 2017 08:54

aduth mentioned this pull request Nov 2, 2017

Parser: Allow block nesting #2743

Merged

jorgefilipecosta closed this Nov 16, 2017

gziolo deleted the add/button-cover-image branch May 7, 2018 11:15

Conversation

jorgefilipecosta commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Screenshots:

Notes:

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtias commented Oct 18, 2017

Uh oh!

codecov bot commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jorgefilipecosta commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Oct 19, 2017

Uh oh!

jorgefilipecosta commented Oct 23, 2017

Uh oh!

mtias commented Oct 24, 2017

Uh oh!

mtias commented Oct 24, 2017

Uh oh!

ellatrix commented Oct 24, 2017

Uh oh!

karmatosed commented Oct 24, 2017

Uh oh!

jorgefilipecosta commented Oct 25, 2017

Uh oh!

jorgefilipecosta commented Oct 31, 2017

Uh oh!

jorgefilipecosta commented Nov 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

jorgefilipecosta commented Oct 18, 2017 •

edited

Loading

codecov bot commented Oct 19, 2017 •

edited

Loading

jorgefilipecosta commented Oct 19, 2017 •

edited

Loading

jorgefilipecosta Oct 23, 2017 •

edited

Loading