-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[Backport] Add wp_theme_element_class_name()
alias
#3244
[Backport] Add wp_theme_element_class_name()
alias
#3244
Conversation
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.
Looks good!
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.
Just two minor suggestions 🙂
src/wp-includes/theme.php
Outdated
* | ||
* @return string The name of the class. | ||
* | ||
* @since 6.1.0 |
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.
Minor: @param
and @return
should come after @since
.
src/wp-includes/theme.php
Outdated
* | ||
* @return string The name of the class. | ||
*/ | ||
function wp_theme_element_class_name( $element ) { |
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.
For consistency with the original method, and for consistent use of the active verb, I'd suggest renaming this alias to wp_theme_get_element_class_name()
. 🙂
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.
Makes sense! That does have a nicer ring to it 😊
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.
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.
LGTM 👍
Since the unit test errors seem spurious (see), I'll try rebasing to kick them off again. |
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
06618d5
to
4c85024
Compare
Committed in https://core.trac.wordpress.org/changeset/54174 @ockham I'll let you close this PR if everything is ok :) |
Oops, I closed it accidentally :D |
No worries @audrasjb 😄 Thank you for committing! |
Add
wp_theme_element_class_name()
alias for the "internal"WP_Theme_JSON::get_element_class_name()
.Backport of @c4rl0sbr4v0s' WordPress/gutenberg#44099.
See this discussion for the rationale.
Note that I've opted not to replace calls to
WP_Theme_JSON::get_element_class_name
in the tests -- since those are specifically for theWP_Theme_JSON
class.Trac ticket: https://core.trac.wordpress.org/ticket/56467
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.