Fixed background grey of the Shortcode block placeholder screen #14719

mapk · 2019-03-30T03:36:22Z

Description

Changed the background grey of the shortcode block to match all the other placeholder setup screens on other blocks. Fixes #14718

How has this been tested?

Tested locally.

Before

After

Types of changes

Changed the variable used for the background grey.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

paulwilde · 2019-03-30T14:06:13Z

It would also make sense to add support for dark themes to be even more consistent with the placeholder component:

.is-dark-theme & {
    background: $light-opacity-light-200;
}

mapk · 2019-04-01T18:51:35Z

Here's how it looks now.

mapk · 2019-04-01T19:49:22Z

Also, the dark mode work for the setup screen.

There is a problem about the placeholder text being lost in the field, but this applies to multiple blocks which should be addressed in another PR.

…ther placeholder setup screens on other blocks. Fixes #14718

@gziolo

…ince @gziolo made some changes.

mapk · 2019-04-05T19:44:48Z

@gziolo Can you review this for me? I had made some changes, then noticed some conflicts with changes you had made earlier. I rebased and force pushed this bad boy, and want to make sure I did it all right. If we're good, can I get your approval?

gziolo

Can you review this for me? I had made some changes, then noticed some conflicts with changes you had made earlier. I rebased and force pushed this bad boy, and want to make sure I did it all right. If we're good, can I get your approval?

All good 👍 There are more changes coming to all core blocks. We need those refactorings to validate all the assumptions made in RFC (#13693).

gziolo · 2019-04-08T07:33:59Z

packages/block-library/src/shortcode/edit.js

@@ -10,8 +10,8 @@ const ShortcodeEdit = ( { attributes, setAttributes, instanceId } ) => {
 	const inputId = `blocks-shortcode-input-${ instanceId }`;

 	return (
-		<div className="wp-block-shortcode">
-			<label htmlFor={ inputId }>
+		<div className="wp-block-shortcode  components-placeholder">


@jasmussen - can you advice how it could be coded differently? In general, we assume that class names are local to the component which define them. In this case, it's wp.components.Placeholder which uses components-placeholder. While it seems to be very convenient to use an existing class, it doesn't scale well as we learned down the road.

Yeah this is an interesting question.

I would say there are two ways to go.

Keep the shortcode block as its own thing. But then we remove the placeholder classes and duplicate the CSS rules necessary for them to look the same.

Rewrite/potentially redesign the shortcode to actually use the placeholder component.

2 is a bigger task, and I have no strong opinions on which one is the right way to go. What do you think?

Well, I'm seeing that components-placeholder* classes are used in several places for other core blocks as well :(

@youknowriad or @aduth - what would be the best way moving forward to tackle it? We definitely need to clean it up.

What is a placeholder component? What does it mean? Does it make sense as a separate component?

One of the main issues with duplicating the class like this is maintainability. If someone wants to refactor the placeholder styles, they'd generally expect only the <Placeholder> component to be affected, but in this case other components can be affected in an unexpected way.

What is a placeholder component?

In the context of a block, it seems like it's the UI that's displayed when the proper wysiwyg part of a block can't be shown. Outside of the context of a block, I'm not really sure. The word 'placeholder' generally implies a stopgap that will eventually be replaced, but I'm not sure how that applies to other user interfaces that @wordpress/components might be used in.

A couple of other options might be:

Extend Placeholder to support other layouts, like the horizontal layout shown here

Extract the container of the placeholder into a separate component (PlaceholderCard?) and use that.

gziolo

Looks good.

We should open a follow-up issue where we cover the part about reusing class names from existing components.

@gziolo

…Press#14719) * Changed the background grey of the shortcode block to match all the other placeholder setup screens on other blocks. Fixes WordPress#14718 * Restyled shortcode block to match placeholder block styling. * Rebased and force pushed to catch up with changes made to this file since @gziolo made some changes.

youknowriad · 2019-04-18T17:07:07Z

I wonder if this is related to these changes here #15032

aduth · 2019-04-18T20:48:59Z

We should open a follow-up issue where we cover the part about reusing class names from existing components.

@youknowriad or @aduth - what would be the best way moving forward to tackle it? We definitely need to clean it up.

Sorry I missed this until now. I agree this shouldn't be done this way, and was part of the reason for #14556 in more strongly asserting that the class name should never be used outside its own component.

A component's class name should never be used outside its own folder (with rare exceptions such as _z-index.scss). If you need to inherit styles of another component in your own components, you should render an instance of that other component. At worst, you should duplicate the styles within your own component's stylesheet. This is intended to improve maintainability by treating individual components as the isolated abstract interface.

I would much rather have seen the styles duplicated, or to make some separate component which encompasses what it is we're trying to do with this padded gray container. But to me, the "padded gray container" is what the Placeholder component is trying to be. Whether it supports the desired use-case, it either (a) should be made to or (b) the desired use-case should be changed to align to what the Placeholder is meant to be used for.

Is it not enough to do something like:

<Placeholder
	icon="shortcode"
	label={ __( 'Shortcode' ) }
>
	<PlainText /* ... */ />
</Placeholder>

mapk self-assigned this Mar 30, 2019

mapk requested a review from chrisvanpatten as a code owner March 30, 2019 03:36

mapk requested review from ajitbohra, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners April 1, 2019 18:50

Soean added the [Feature] Shortcodes Related to shortcode functionality label Apr 1, 2019

mapk added 3 commits April 5, 2019 12:35

Changed the background grey of the shortcode block to match all the o…

a7555eb

…ther placeholder setup screens on other blocks. Fixes #14718

Restyled shortcode block to match placeholder block styling.

aaa9e3a

Rebased and force pushed to catch up with changes made to this file s…

1c67d6c

…ince @gziolo made some changes.

mapk force-pushed the update/shortcode-grey branch from a595461 to 1c67d6c Compare April 5, 2019 19:42

gziolo reviewed Apr 8, 2019

View reviewed changes

gziolo requested a review from jasmussen April 8, 2019 07:30

gziolo reviewed Apr 8, 2019

View reviewed changes

gziolo approved these changes Apr 10, 2019

View reviewed changes

mapk merged commit 17f8d86 into master Apr 10, 2019

youknowriad deleted the update/shortcode-grey branch April 11, 2019 07:18

youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

This was referenced Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

Bump/5.5 #15011

Closed

This was referenced Apr 17, 2019

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

hustlahusky mentioned this pull request Apr 18, 2019

Why shortcode block outputs as oneline textarea in 5.5.0? #15032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed background grey of the Shortcode block placeholder screen #14719

Fixed background grey of the Shortcode block placeholder screen #14719

mapk commented Mar 30, 2019 •

edited

Loading

paulwilde commented Mar 30, 2019

mapk commented Apr 1, 2019

mapk commented Apr 1, 2019

mapk commented Apr 5, 2019

gziolo left a comment

gziolo Apr 8, 2019

jasmussen Apr 8, 2019

gziolo Apr 8, 2019 •

edited

Loading

youknowriad Apr 8, 2019

talldan Apr 9, 2019

gziolo left a comment •

edited

Loading

youknowriad commented Apr 18, 2019

aduth commented Apr 18, 2019

Fixed background grey of the Shortcode block placeholder screen #14719

Fixed background grey of the Shortcode block placeholder screen #14719

Conversation

mapk commented Mar 30, 2019 • edited Loading

Description

How has this been tested?

Before

After

Types of changes

Checklist:

paulwilde commented Mar 30, 2019

mapk commented Apr 1, 2019

mapk commented Apr 1, 2019

mapk commented Apr 5, 2019

gziolo left a comment

Choose a reason for hiding this comment

gziolo Apr 8, 2019

Choose a reason for hiding this comment

jasmussen Apr 8, 2019

Choose a reason for hiding this comment

gziolo Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

youknowriad Apr 8, 2019

Choose a reason for hiding this comment

talldan Apr 9, 2019

Choose a reason for hiding this comment

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

youknowriad commented Apr 18, 2019

aduth commented Apr 18, 2019

mapk commented Mar 30, 2019 •

edited

Loading

gziolo Apr 8, 2019 •

edited

Loading

gziolo left a comment •

edited

Loading