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

Update site editor block placeholder styling #35513

Merged
merged 8 commits into from
Oct 14, 2021
Merged

Update site editor block placeholder styling #35513

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
33c960c
Update styling
jameskoster Oct 11, 2021
0b7a014
Post author weight, fix imports
jameskoster Oct 11, 2021
32e3f01
increase featured image height
jameskoster Oct 11, 2021
a76a286
Revert featured image changes
jameskoster Oct 11, 2021
d3baa94
Merge branch 'trunk' into update/theme-block-placeholder-styling
jameskoster Oct 12, 2021
9c0b90e
Merge branch 'trunk' into update/theme-block-placeholder-styling
jameskoster Oct 12, 2021
dc8a095
Formatting
jameskoster Oct 12, 2021
ea8173f
Remove editor styles for Post Author
jameskoster Oct 13, 2021
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
2 changes: 0 additions & 2 deletions packages/block-library/src/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,11 @@
@import "./template-part/editor.scss";
@import "./text-columns/editor.scss";
@import "./video/editor.scss";
@import "./query-title/editor.scss";
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that we can remove all of these single-purpose stylesheets.

@import "./post-template/editor.scss";
@import "./query/editor.scss";
@import "./query-pagination/editor.scss";
@import "./query-pagination-numbers/editor.scss";
@import "./post-featured-image/editor.scss";
@import "./term-description/editor.scss";

:root .editor-styles-wrapper {
@include background-colors-deprecated();
Expand Down
1 change: 0 additions & 1 deletion packages/block-library/src/post-author/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

.wp-block-post-author__name {
margin: 0;
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize this PR is not about the Post Author block. But seeing just this one font weight removed makes me wonder: why are there so many rules just for the editor view of the post author block? It seems like it would cause a lot of divergence. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's because the Post Author block currently also includes an option to display the author avatar, bio, and a byline (I'm not entirely sure where the byline comes from 😬). These should all be separate blocks, but that is clearly another issue :D

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, whether one or multiple. My question is rather, why are these styles stored in editor.scss rather than style.scss. The latter is loaded in both the editor and the frontend, the former is only for editor specific affordances. Was just surprised by how many such there are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. Yes that does seem odd. I'll see if I can tidy 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.

Well, it doesn't have to be this PR, or you — I mostly noticed it because you tweaked the font weight, making me wonder: why just the font weight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we don't need the editor-specific styles for the Post Author block at all.

I think we can tidy these styles up even more when we split this in to separate blocks.

}

.wp-block-post-author__bio {
Expand Down
4 changes: 1 addition & 3 deletions packages/block-library/src/post-excerpt/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@ export default function PostExcerptEditor( {
if ( ! postType || ! postId ) {
return (
<div { ...blockProps }>
<Warning>
{ __( 'Post excerpt block: no post found.' ) }
</Warning>
{ __( 'Post excerpt block: no post found.' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good enough to me. What did you think of just showing the block name?

That said, I'm losing a bit of orientation when using this one right now:
excerpt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this occurs when you're editing content in the site editor, and the content that you're editing has no excerpt, but the template attempts to display it.

I just checked and it turns out this issue exists on trunk. I think it's a separate thing, so we can probably create a dedicated issue.

What did you think of just showing the block name?

Totally, I was going to do the content stuff in a separate PR, but I suppose we could do it here.

</div>
);
}
Expand Down
22 changes: 17 additions & 5 deletions packages/block-library/src/post-featured-image/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
import { useEntityProp, store as coreStore } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';
import {
Icon,
ToggleControl,
PanelBody,
Placeholder,
withNotices,
} from '@wordpress/components';
import {
Expand All @@ -19,6 +19,7 @@ import {
} from '@wordpress/block-editor';
import { __, sprintf } from '@wordpress/i18n';
import { postFeaturedImage } from '@wordpress/icons';
import { SVG, Path } from '@wordpress/primitives';

/**
* Internal dependencies
Expand All @@ -27,10 +28,21 @@ import DimensionControls from './dimension-controls';

const ALLOWED_MEDIA_TYPES = [ 'image' ];
const placeholderChip = (
<div className="post-featured-image_placeholder">
<Icon icon={ postFeaturedImage } />
<p> { __( 'Featured Image' ) }</p>
</div>
<Placeholder className={ 'block-editor-media-placeholder' }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm impressed you took this one on directly! I do think maybe this one might be better as a separate PR, as it's likely more of a mouthful than the first glance. For example, it's not looking too great in the post editor:
Screenshot 2021-10-11 at 15 26 14

There's a discussion on what role it serves in the post editor, but nevertheless it is there at the moment.

I also think we'll either want to tweak the mountainrange illustration, or make it scale to fill the entire canvas rather than just be centered in the middle:
Screenshot 2021-10-11 at 15 26 45

{
<SVG
className="components-placeholder__illustration"
fill="none"
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 60 60"
>
<Path
vectorEffect="non-scaling-stroke"
d="m61 32.622-13.555-9.137-15.888 9.859a5 5 0 0 1-5.386-.073l-9.095-5.989L1 37.5"
/>
</SVG>
}
</Placeholder>
);

function PostFeaturedImageDisplay( {
Expand Down
66 changes: 46 additions & 20 deletions packages/block-library/src/post-featured-image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,55 @@ div[data-type="core/post-featured-image"] {
}
}

.editor-styles-wrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nice to get rid of, though.

.post-featured-image_placeholder {
display: flex;
flex-direction: row;
align-items: center;
border-radius: $radius-block-ui;
background-color: $white;
box-shadow: inset 0 0 0 $border-width $gray-900;
padding: $grid-unit-15;
svg {
margin-right: $grid-unit-15;
}
p {
font-family: $default-font;
font-size: $default-font-size;
margin: 0;
}
}
}

.block-library-post-featured-image-dimension-controls {
margin-bottom: $grid-unit-10;
&.scale-control-is-visible {
margin-bottom: $grid-unit-20;
}
}
.wp-block-post-featured-image {
.components-placeholder {
justify-content: center;
align-items: center;
box-shadow: none;
padding: 0;

// Provide a minimum size for the placeholder, for when the logo is resized.
// @todo: resizing is currently only possible by adding an image, resizing,
// and then removing the image again. We might want to enable resizing on the
// placeholder itself.
min-height: $grid-unit-60;
min-width: $grid-unit-60;
height: $grid-unit * 30;
width: 100%;

&::before {
content: "";
display: block;
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
border: $border-width dashed currentColor;
opacity: 0.3;
pointer-events: none;

// Inherit border radius from style variations.
border-radius: inherit;
}
}

.components-placeholder__illustration {
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
width: 100%;
height: 100%;
stroke: currentColor;
stroke-dasharray: 3;
opacity: 0.3;
}
}
4 changes: 0 additions & 4 deletions packages/block-library/src/query-title/editor.scss
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions packages/block-library/src/term-description/editor.scss
View file
Open in desktop

This file was deleted.