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.

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

HTML API: Only pass a single class name to add_class() #5325

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Sep 27, 2023

In some places, multiple classes are being passed to the Tag Processor's add_class() method. They are being passed as a string containing class names separated by whitespace. This produces the expected behavior because the Tag Processor doesn't enforce its expectation in the add_class() method, but allowing this can lead to mistakes when interacting with classes.

In this patch these places are updated to only add a single class at a time, and a helper CSS class is created with a commonly-used method class_list() to split such combined strings.

The Tag Processor could be changed so that add_class() splits CSS strings implicitly, but that introduces a hidden form of runtime overhead that may not be evident from the name. It also leads to confusing code patterns, as seen in this patch, where plurality mismatches appear: add_class( $classes )

Another method, add_classes() could be introduced in the Tag Processor, and that could split the class names, or it could accept an array of tag names. It's not clear which of these would be more useful, clear, or likely to avoid mistakes.

For now, by leaning into the existing API definitions it's probably best to clarify expectations and behaviors rather than to start blurrying definitions and interfaces, and that's what this patch proposes doing.

In some places, multiple classes are being passed to the Tag Processor's
`add_class()` method. They are being passed as a string containing class
names separated by whitespace. This produces the expected behavior because
the Tag Processor doesn't enforce its expectation in the `add_class()`
method, but allowing this can lead to mistakes when interacting with
classes.

In this patch these places are updated to only add a single class at a
time, and a helper CSS class is created with a commonly-used method
`class_list()` to split such combined strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in Slack, this will need changing in Gutenberg; it will then require a new release of the @wordpress/block-library npm so it can be finally sync'ed to Core. (There's currently no other way 😅 )

AFAICT, you've started work on this already in WordPress/gutenberg#54873. If we want to get this into WP 6.4, we'd need to land it fairly soon so it can be cherry-picked for one of the next rounds of npm releases (probably prior to Beta 3 on Oct 10).

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the review. I don't see a need to push this into 6.4 as it doesn't currently cause any problems (beyond the fact that passing mutiple class names can cause problems), but it will need to be updated before we lock down adding multiple classes probably for 6.5

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

As mentioned, we'd have to remove the change to the dynamic block code from the PR (and land that separately, by way of a package sync).

It seems like the other code can stand alone. Would you like this in 6.4? I'm not 100% sure that it qualifies as a bugfix, which would be the criterion (unless we can make it a "blessed task") 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants