Revert Site Tagline placeholder attribute, move example to block.json #48383
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] Code Quality
andrewserong
requested review from
Mamaduka,
apeatling,
tellthemachines and
ntsekouras
andrewserong
requested review from
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: -18 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 42d94dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4257422541
|
tellthemachines
approved these changes
Feb 24, 2023
| "textAlign": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "example": {}, |
There was a problem hiding this comment.
So is this the new canonical way to set block examples or are they only added to block.json for certain cases? All the ones I can see in trunk seem to be dynamic blocks.
There was a problem hiding this comment.
Thanks for reviewing! I wasn't too sure what's the preferred approach, they both seem to work, and I was mostly copying from the example over in #48326. However, the reason I'd included example in index.js originally is that we can wrap any strings in translation calls, so that might be a reason why many of them use index.js instead (e.g. Buttons block's example has strings for the text attribute, etc)
@ntsekouras do you have a preference for where example goes?
There was a problem hiding this comment.
I think is better to have properties in block.json, when we can. There have been a lot of work from @gziolo in this area.
There was a problem hiding this comment.
You can't express everything in block.json with the example field, e.g. translatable strings. I don't know whether I have a preference here.
|
No worries, thanks for the reviews! |
andrewserong
added
the
Backport to WP Beta/RC
|
Nice work getting a quick change in @andrewserong 👍 |
ntsekouras
pushed a commit
that referenced
this pull request
Feb 27, 2023
…#48383) * Revert Site Tagline placeholder attribute, move example to block.json * Remove from docs * Fix bad commit
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: b8ecf22 |
ntsekouras
removed
the
Backport to WP Beta/RC
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?
Revert Site tagline block's
placeholderattribute as requested in this review comment: #48300 (comment)Why?
The concern is that introducing a new attribute just for the preview state is not worth the future support or deprecation work should there be changes in the future. Also, reverting appears to be pretty safe, and doesn't prevent us from exploring adding back in some form of custom placeholder info in a future PR if there's feedback about the
Write site tagline...text being confusing.How?
placeholderattribute and code that was introduced in Site Tagline: Add example so that it will display in style book #48300exampleso that it follows the approach from [Block Library - Site Tagline]: Add example in block settings #48326Note: since the
placeholderattribute never affects any markup saved to post content (and this is a server-rendered block), we don't have to worry about deprecations.Testing Instructions
Test markup:
<!-- wp:site-tagline {"placeholder":"My custom placeholder..."} /-->Testing steps:
My custom placeholder...Write site tagline…placeholder text should be shown instead, and there should be no errors in the console. If you go to the code editor view, theplaceholderattribute should no longer be present.Write site tagline...placeholder should show (or if you have a Site Tagline set, then the current text should be displayed in the preview)Screenshots or screencast