Make WordPress Core

Opened 7 years ago

Last modified 5 weeks ago

#44923 reviewing enhancement

Filter .children class on nested comments list

Reported by: greenshady's profile greenshady Owned by: davidbaumwald's profile davidbaumwald
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch dev-feedback
Focuses: css Cc:

Description

The start_lvl() method of Walker_Comment should have a filter hook to customize the .children class. This will allow theme authors to utilize their preferred system or CSS framework for handling nested comment lists. And, .children is generic and requires writing nested CSS selectors to target it.

This change would put the comment walker on par with the nav menu walker, where we have the nav_menu_submenu_css_class filter hook.

Attachments (2)

44923.patch (953 bytes) - added by Shizumi 7 years ago.
44923.2.patch (1.3 KB) - added by Shizumi 7 years ago.

Download all attachments as: .zip

Change History (13)

This ticket was mentioned in Slack in #themereview by greenshady. View the logs.


7 years ago

#2 @joyously
7 years ago

I'm all for parity, but when I tried to make my menu work with CSS only, I had to insert elements at the parent level of the submenu, not at the child level. So while this filter would be good for comments, it would have the same problem of not being able to inject something at the parent level, which is how you have to work with CSS.

In my case, I had to supply an entire nav walker class just to output a small piece of HTML to be able to trigger the submenus. Comments are a bit less dynamic, but same nested lists.

What I'm saying is: is a filter enough?

#3 @greenshady
7 years ago

Yes, a filter is plenty when all you need to do is alter a class.

It sounds like the issues you're describing are out of scope for this proposed enhancement.

#4 @SergeyBiryukov
7 years ago

  • Keywords needs-patch added

@Shizumi
7 years ago

#5 @Shizumi
7 years ago

I made a patch.
Because it is the first time, please let me know if there is a shortage.

#6 @greenshady
7 years ago

The filter hook might be more appropriately named comment_list_sublist_class or something to that effect. Other ideas?

Also, $class_names needs to be wrapped in esc_attr() when adding to the HTML.

@Shizumi
7 years ago

#7 @Shizumi
7 years ago

Thank you for your comment.
I tried to rewrite it.

#8 @kirasong
6 weeks ago

  • Focuses css added
  • Keywords has-patch dev-feedback added; needs-patch removed

#9 @kadamwhite
6 weeks ago

Thank you for the patch. At WC Kansai contributor day we've discussed this, and aren't sure the best person to endorse inclusion of a new filter. tagging @davidbaumwald for opinions as the listed Comments maintainer. (Any change would be for 7.0 or later, at this point in the 6.9 cycle)

@Shizumi Thank you for your patch. My feedback is that including class="" in the filter makes it harder to escape; if you had the output code look like this,

<ol class="<?php echo esc_attr( $class_names ); ?>">

and then had the filter only apply to the string of class names, it would be simpler.

#10 @dmsnell
6 weeks ago

This comment doesn’t speak to the value or tradeoffs involve in creating this new filter, but I can propose one suggestion to avoid further HTML escaping issues, and that is to create the opening tag using the Tag Processor.

<?php
if ( 'div' === $args['style'] ) {
        return;
}

$class_names = apply_filters( 'comment_list_sublist_class', $classes, $args, $depth );

$html = 'ol' === $args['style'] ? '<ol>' : '<ul>';

if ( ! $class_names ) {
        return $html;
}

$processor = new WP_HTML_Tag_Processor( $html );
$processor->next_token();
foreach ( $class_names as $class_name ) {
        $processor->add_class( $class_name );
}

return $processor->get_updated_html();

This can also work, of course, if the filter output is a list of strings, but the array form is convenient and avoids syntax particulars about separating CSS class names (something explored in wordpress/wordpress-develop#10043 and in #63694)

#11 @davidbaumwald
5 weeks ago

  • Milestone changed from Awaiting Review to 7.0
  • Owner set to davidbaumwald
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.