Improve do_settings_fields() by including field ID on <tr> table row

[Jonnyauk]

It would be very useful to target individual table rows (for Javascript/CSS manipulation) as generated using the WP settings API - but when looking at the source and do_settings_fields() it just has a hardcoded <tr> opening tag.

I could search through using something like JQuery and see what's inside the table row - but this seems unnecessary when it would be more useful to add the ID of the settings 'field' straight into the <tr> tag as a class that can then be easily targeted.

Please see submitted patch - small modification to do_settings_fields() that adds a CSS class so it can be easily targeted ;) I didn't use ID just in-case of multiple settings field ID's and therefore none-unique ID's (which surely would be bad practice and not function correctly using the WP Settings API anyway?!)

[Jonnyauk]

do_settings_fields() add class to <tr> (field ID)

[nacin]

As indicated in #28754, I'm not sure I'd want to do anything that ties us closer to the table markup we have here. The "hack" that is grabbing the field then traversing to find a common parent of itself and a label (or something) at least feels like a hack, that way we can feel less bad when it breaks.

[Jonnyauk]

Thanks for the feedback - I'm a bit rubbish at JS - but sure that I can figure out how to traverse the markup to target what I need to manipulate and understand that this marked for future development (and improvement) - so will keep an eye on the settings API (and hopefully maybe help out if I'm able!)

[valendesigns]

@nacin I've updated the patch slightly. Instead of adding the field ID as a class every time, which is not very extensible, I made it look for the value of the class parameter, which would have been passed through the $args array in add_settings_field(). It's the same way label_for is passed around.

I'm not sure whether you still feel this would tie us to the markup. But if you change you're mind on that front, I think explicitly setting the class for each element is a better solution.

For example, say you've added a few settings to the profile page and one of them is to activate and make use of the other settings (something similar to show_avatars). When you activate the main setting it would be much easier to add JavaScript to toggle the other options visually in the UI with this patch.

What brought this to my attention, and was why I had planned to create my own ticket before I found this one, is that Mark created ticket #30168 that would visually toggle the avatar settings. If that patch makes it into the core and you want to seamlessly add additional avatar settings that will also be toggled you would need to duplicate his JavaScript. I noticed this when I was working on a better way to cache Gravatars than the plugins that are currently available. A whole different issue, but how I ended up here.

Also, I have a second patch that cleans up that functions PHP and removes my ternary operator to be more inline with the coding standards, but figured I would wait to hear back from you first. My first patch is to highlight the change being requested.

I'm just asking for a reconsideration before this ticket is closed.

Cheers,
Derek

[valendesigns]

I've refreshed the patch and cleaned up the code to be inline with the standards. Now that #30168 is in Core we cannot add additional avatar settings to the UI in options-discussion.php that are automatically toggled on and off without applying this patch. For example, lets say I wanted to create an avatar caching plugin and add a setting right after the other avatar settings. When someone toggles those settings my custom one will still be displayed, even though I want it to be hidden. The only recourse I have is to duplicate the JS and cobble together a solution without a clearly defined CSS selector.

Additionally, we really do need a way to add some sort of specificity to the tr or div or whatever future markup we use beyond the avatar settings issue. In order to easily manipulate the DOM with JS there has to be a class or ID we can select, or we end up searching and guessing which is not consistent enough.

[helen]

I'm okay with this, especially in light of #30168. I don't think it ties us too much to the markup elements - short of predicting the future, as far as I can see any changes would still include a wrapper element around a given field group. If somebody starts targeting things in ways that are tied to the specific structure, I don't see future breakage being catastrophic, plus there are plenty of other things that would likely break that we can't help anyway, so plugin/theme updates would be a necessity.

Going to simplify to avoid the first else, but otherwise looks good.

[helen]

In 31560:

Settings API: Allow passing a class to add_settings_field() via the $args array.

While it's possible to target the wrapper element otherwise (currently a tr), this deficiency is made especially noticeable when custom code cannot take advantage of what core is doing, such as with avatars in #30168.

props valendesigns.
fixes #28975. see #30168.

[valendesigns]

@helen Thank you for move this one forward. I'm glad to see it made it in before the 4.2 enhancement cutoff date. Cheers!

[DrewAPicture]

A changelog entry should've been added to the add_settings_field() DocBlock for the new class argument.

In 28975.diff​, I've added a hash notation for the $args parameter in add_settings_field() with descriptions for both the label_for and new class arguments. It also adds the changelog entry and fixes the syntax of the other parameter descriptions in the DocBlock.

[DrewAPicture]

In 31592:

Add a hash notation for the optional $args parameter in add_settings_field(), which includes a description for the new $class argument added in [31560].

Also adds a changelog entry to add_settings_field() DocBlock for the new $class argument.

See [31560].
Fixes #28975.

[DrewAPicture]

In 31593:

Fix a typo in the $args parameter hash notation description for add_settings_field().

See #28975, [31592].