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

Site Tagline: Add example so that it will display in style book #48300

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Site Tagline: Add example so that it will display in style book #48300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2694213
Site Tagline: Add example so that it will display in style book
andrewserong Feb 22, 2023
94f8dfb
Add placeholder attribute
andrewserong Feb 22, 2023
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: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ Describe in a few words what the site is about. The tagline can be used in searc
- **Name:** core/site-tagline
- **Category:** theme
- **Supports:** align (full, wide), anchor, color (background, gradients, text), spacing (margin, padding), typography (fontSize, lineHeight), ~~html~~
- **Attributes:** textAlign
- **Attributes:** placeholder, textAlign

## Site Title

Expand Down
3 changes: 3 additions & 0 deletions packages/block-library/src/site-tagline/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
"keywords": [ "description" ],
"textdomain": "default",
"attributes": {
"placeholder": {
Copy link
Contributor

@ntsekouras ntsekouras Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really sure we should add a new block attribute just for preview purposes.. I think we should revert this change.

IMO it's not worth it to have to support this and deprecate it in the future, for just showing something different in previews in the case a site doesn't have one. I mean Write site tagline… isn't so bad, is it?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @ntsekouras. Having attribute just for the block previews seems odd to me.

Let's follow the example of the Site Title and Site Logo blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @ntsekouras and @Mamaduka! Here's how it would look reverted:

Just to share my thinking about introducing it: I don't feel too strongly about it, but my main concern with not having a placeholder value is that it could be fairly common for folks to not have a site tagline, but very uncommon to not have a site title, so there's a higher likelihood that the Write site tagline... text will display in the preview within the Style Book. If a user goes to click that, then they'll be taken to global styles, and they're not able to edit it. We already have the placeholder attribute and similar behaviour on the button, heading, image, list, list item, and paragraph blocks, so I thought it'd be fairly innocuous.

That said, I very much agree that it's good not to have extra attributes if they're not needed, and by reverting there's nothing stopping us from tweaking how previews work in future follow-ups, so I'm happy for us to proceed with reverting. I have a PR open for that over in #48383.

Copy link
Member

Choose a reason for hiding this comment

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

I agree UX might be confusing with the default placeholder, but maybe this is something we need to resolve separately. Maybe change a text copy.

Fun fact: WP only removed the default tagline a few months ago, and it broke our E2E tests 😄

Copy link
Contributor Author

@andrewserong andrewserong Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

but maybe this is something we need to resolve separately. Maybe change a text copy.

I agree — looking over this again today, I wasn't 100% sold on the placeholder approach as the preview should really be previewing a finished / proper Site Tagline rather than the placeholder state since the placeholder has reduced opacity. This is likely pretty low priority, though, so we could always wait and see if there's any feedback about the preview state.

Fun fact: WP only removed the default tagline a few months ago, and it broke our E2E tests 😄

Whoa! I didn't know that, good to know 😄

Copy link
Member

Choose a reason for hiding this comment

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

I agree on just using the placeholder the block provides if the site doesn't have a site tagline set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming!

"type": "string"
},
"textAlign": {
"type": "string"
}
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/site-tagline/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default function SiteTaglineEdit( {
setAttributes,
insertBlocksAfter,
} ) {
const { textAlign } = attributes;
const { placeholder, textAlign } = attributes;
const { canUserEdit, tagline } = useSelect( ( select ) => {
const { canUser, getEntityRecord, getEditedEntityRecord } =
select( coreStore );
Expand Down Expand Up @@ -57,7 +57,7 @@ export default function SiteTaglineEdit( {
allowedFormats={ [] }
onChange={ setTagline }
aria-label={ __( 'Site tagline text' ) }
placeholder={ __( 'Write site tagline…' ) }
placeholder={ placeholder || __( 'Write site tagline…' ) }
tagName="p"
value={ tagline }
disableLineBreaks
Expand Down
10 changes: 10 additions & 0 deletions packages/block-library/src/site-tagline/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
Expand All @@ -13,6 +18,11 @@ export { metadata, name };
export const settings = {
icon,
edit,
example: {
attributes: {
placeholder: __( 'A site tagline.' ),
},
},
deprecated,
};

Expand Down