-
Notifications
You must be signed in to change notification settings - Fork 865
Updated Action Hooks and Classes for Wordpress Coding Standards #298
Conversation
Also fixed a couple “yoda condition” issues to make unit testing pass.
Just added a reminder for people to change the status badge to point to their specific repository unit test.
|
||
function display_element( $element, &$children_elements, $max_depth, $depth = 0, $args, &$output ) { | ||
$element->has_children = ! empty( $children_elements[ $element->ID ] ); | ||
$element->classes[] = ( $element->current || $element->current_item_ancestor ) ? 'active' : ''; | ||
$element->classes[] = ( $element->has_children && $max_depth !== 1 ) ? 'has-submenu' : ''; | ||
$element->classes[] = ( 1 !== $element->has_children && $max_depth ) ? 'has-submenu' : ''; |
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.
This change seems wrong, previously it was
$element->has_children && $max_depth !== 1
and now it is
1 !== $element->has_children && $max_depth
shouldn't it rather be
$element->has_children && 1 !== $max_depth
?
Also, and I feel like I am nitpicking here, but if we are to follow coding standards then The WordPress Naming Conventions say this about naming classes:
|
Also fixed an error in menu-walker.php and offcanvas-walker.php
@Aetles you were totally right. I completely missed that in the menu walkers. It had actually happened in menu-walker.php as well, but I went ahead and changed it. Both are now fixed and read: $element->has_children && 1 !== $max_depth Also thanks for reminding me of the true WP Standards definition for classes. I don't know why I was under the impression that it Classes were to be camel-case. Either way, it should all be changed now. I think I hit all the classes, but might need to have a double check on that. The only thing that remains in question in my mind would be the text domain. WordPress doesn't seem to have a standard for the text-domain. However in the I18n Documentation found here it does state the following under the text domain section:
So I guess my last question is if we need to change the |
Great! About the text domain: I'd vote for changing |
@olefredrik What do you think about the text domain change? |
Updated Action Hooks and Classes for Wordpress Coding Standards
I agree that it could be okay to change the text-domain to |
Also fixed a couple “yoda condition” issues to make unit testing pass.