-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Block Library: Refactor core blocks to use HTML Tag Processor #43178
Conversation
a8a5b3b
to
4887c5c
Compare
7e4ce38
to
5882144
Compare
5882144
to
9a76c76
Compare
9a76c76
to
f12f8ed
Compare
I rebase this branch with I wrapped all values passed to |
Esc_attr shouldn’t be needed thanks to #44447. Let’s remove it to avoid escaping the value twice. |
Nice, it still might be necessary to use some sanitization in some cases so we need to have a good documentation that covers all possible scenarios. |
); | ||
$processor = new WP_HTML_Tag_Processor( $content ); | ||
$processor->next_tag(); | ||
$processor->set_attribute( 'style', $styles ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would $tags
or $tag_stream
be more useful names here? I think we discussed this at one point. seeing $processor
here reads to me as if things are more complicated than they are.
$tag_stream = new WP_HTML_Tag_Processor( $content );
$tag_stream->next_tag();
$tag_stream->set_attribute( 'style', $styles );
$content = (string) $tag_stream;
not that I care how people name the instance of this class, but given that it's core code I recognize that we're setting the examples and how we do it will influence how people use it; I want our examples to be super clear. (not saying $tag_stream
is universally clearer, just raising the point to discuss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking. It's one of the reasons I wanted to have this PR opened early on so we can exercise this API in the codebase.
We still have some time to apply changes to the API or how we will promote usage. From the reader's perspective, I might prefer:
$tag_stream = new WP_HTML_Tag_Stream( $content );
$tag_stream->next();
$tag_stream->set_attribute( 'style', $styles );
$content = (string) $tag_stream;
I changed the name of the class to align with the variable. I also renamed next_tag
to avoid repeating the word tag
twice.
$tags
is shorter, so it might be more desirable. It also feels similar to taxonomy tags, but I don't think they will be used too often together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also renamed next_tag to avoid repeating the word tag twice.
double-check with @adamziel. that seems like a much bigger change since it impacts the API and not only the application-side. if someone does create $p = new WP_HTML_Tag_Processor();
then they'll have $p->next()
in my original proposal I used ->find()
as the query function but @adamziel had feelings about how ->next_tag()
communicated forward progression through the document.
I could be fine with next()
, given that we now have WP_HTML_Tag_Processor
indicating what "next" is, but it does feel slightly sparser in the self-documenting way (and at the same time, repeating "tag" here doesn't feel like any real inconvenience)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to say $tag_stream = new WP_HTML_Tag_Processor();
– I think I'd rather say something like $tags = new WP_HTML_Tag_Processor()
or $tags = new WP_HTML_Tag_Stream()
. The latter doesn't communicate the processing capabilities, though, so I'd go with $tags = new WP_HTML_Tag_Processor()
.
If we use $tags
as the variable name, it makes sense to say ->next()
, but if we don't, then I'd rather say ->next_tag()
. Since we can only suggest these conventions but not enforce them, I feel like ->next_tag()
will be more useful for the larger audience, even if a tiny bit more annoying to type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to leave it as is for now. The following part doesn't look ideal:
$tags->set_attribute( 'foo', 'boo' );
$content = $tags->get_updated_html();
if ( 'home' === $processor->get_attribute( 'rel' ) ) { | ||
$processor->set_attribute( 'aria-label', __( '(Home link, opens in a new tab)' ) ); | ||
$processor->set_attribute( 'target', $attributes['linkTarget'] ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
places like this make me appreciate the new interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in full agreement here 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 indeed
If we want these in WordPress 6.2 we should hurry and merge them, otherwise I don't think it matters if we merge sooner or later. |
5597a30
to
c3a6012
Compare
I refreshed this PR to use the latest API. It doesn't matter if we include it in WordPress 6.2. Actually, it's probably better to test it in the plugin first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks great to me @gziolo, feel free to merge or postpone as you see fit.
c3a6012
to
9bb0a03
Compare
Flaky tests detected in 9bb0a03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4231586927
|
Still looking good 👍 |
I'd like to know if hurry was the only reason the regex for removing the link from the site logo was left as is? |
HTML Tag Processor doesn't support removing HTML tags as of today, it only allows operations on HTML attributes. |
Aha! Thank you @gziolo. We need to wait then. |
What?
The full proposal for the new HTM Tag Processor API is available at https://make.wordpress.org/core/2022/08/19/a-new-system-for-simply-and-reliably-updating-html-attributes/.
Why?
This branch presents the usage of the new HTML Tag Processor API introduced by @adamziel and @dmsnell in #42485.
How?
Updated core blocks:
Testing Instructions
All updated core blocks should work as before.