Site Tagline: Add example so that it will display in style book #48300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
andrewserong
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +20 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 94f8dfb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4239413969
|
apeatling
approved these changes
Feb 22, 2023
|
Thanks for reviewing! |
andrewserong
added
the
Backport to WP Minor Release
andrewserong
added
Backport to WP 6.7 Beta/RC
|
I've added the Backport to WP Beta/RC label since this issue was flagged during testing of the 6.2 beta which introduces the Style Book feature. It's definitely not a showstopping issue, though, if it's now too late to get it in. |
ntsekouras
reviewed
Feb 23, 2023
| @@ -8,6 +8,9 @@ | |||
| "keywords": [ "description" ], | |||
| "textdomain": "default", | |||
| "attributes": { | |||
| "placeholder": { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
I agree on just using the placeholder the block provides if the site doesn't have a site tagline set.
There was a problem hiding this comment.
Thanks for confirming!
30 tasks
ntsekouras
pushed a commit
that referenced
this pull request
Feb 27, 2023
* Site Tagline: Add example so that it will display in style book * Add placeholder attribute
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: efc8ad9 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
17 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add an empty to the Site Tagline block so that it will be displayed in the Style Book in global styles. Also adds a
placeholderattribute so that if the site does not currently have a tagline set, the preview does not say "Write a site tagline..."Why?
Without an
exampleobject set, the block will not be previewed in the Style Book. Much like the Site Title or other theme blocks, it's important that it can be seen / previewed within the Style Book when users are making changes to global styles.How?
exampleobject to the Site Tagline block (using the same approach as the Site Title block). This causes an example of the block to be displayed in the inserter and in the Style Book. No parameters need to be provided in the example as the block will render the current site's tagline, however aplaceholderattribute has been included so that on sites with no tagline set, the preview will not say "Write a site tagline..."Testing Instructions
Screenshots or screencast