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

Fixed background grey of the Shortcode block placeholder screen #14719

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Fixed background grey of the Shortcode block placeholder screen #14719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7555eb
Changed the background grey of the shortcode block to match all the o…
mapk Mar 30, 2019
aaa9e3a
Restyled shortcode block to match placeholder block styling.
mapk Mar 30, 2019
1c67d6c
Rebased and force pushed to catch up with changes made to this file s…
mapk Apr 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/block-library/src/shortcode/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is an interesting question.

I would say there are two ways to go.

  1. 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.
  2. 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?

Copy link
Member

@gziolo gziolo Apr 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Extend Placeholder to support other layouts, like the horizontal layout shown here
  2. Extract the container of the placeholder into a separate component (PlaceholderCard?) and use that.

<label htmlFor={ inputId } className="components-placeholder__label">
<Dashicon icon="shortcode" />
{ __( 'Shortcode' ) }
</label>
Expand Down
8 changes: 5 additions & 3 deletions packages/block-library/src/shortcode/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
.wp-block-shortcode {
display: flex;
flex-direction: row;
flex-direction: column;
padding: $block-padding;
background-color: $light-gray-100;
background-color: $dark-opacity-light-200;
font-size: $default-font-size;
font-family: $default-font;

label {
display: flex;
align-items: center;
margin-right: $grid-size;
white-space: nowrap;
font-weight: 600;
flex-shrink: 0;
}

.block-editor-plain-text {
flex-grow: 1;
line-height: 1;
max-height: 30px;
width: 80%;
}

.dashicon {
Expand Down