#41146 closed enhancement (fixed)
Add filter for a site's class (WP_MS_Users_List_Table)
Reported by: | kraftbj | Owned by: | desrosj |
---|---|---|---|
Milestone: | 5.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Networks and Sites | Keywords: | has-patch |
Focuses: | administration | Cc: |
Description
In the WP_MS_Users_List_Table
, each site that is listed is span
'd with a class of site-#
where #
is the ID of the network for a site.
Adding a filter to this class would allow site administrators to add addition classes to this output which would easily allow for styling of site items based on the additional classes.
Attachments (4)
Change History (19)
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
7 years ago
#2
@
7 years ago
- Keywords needs-patch added; has-patch removed
Thanks for the initial patch @kraftbj. This sounds like a good addition. Some thoughts:
- I think that this filter should pass an array of classes instead of a string. This will help prevent situations where someone adds a class but forgets to add a space, causing classes to bleed together.
- The filter should be on its own line to make it more readable.
- An
array_unique()
call would also help prevent duplicate classes. - Now that there is a filter, whatever value is output into the class attribute will need to be passed through
esc_attr()
.
#3
@
7 years ago
- Keywords has-patch added; needs-patch removed
- Owner set to kraftbj
- Status changed from new to accepted
Made the suggested changes. I'm only checking for is_array
to make sure the return is in the right format. Someone can return false
, etc to short-circuit completely.
We could also check for an empty array, but I don't want to overengineer it.
#4
@
7 years ago
@kraftbj
I'm only checking for
is_array
to make sure the return is in the right format.
I think we can even strip this out, as it's commonly expected to return the same type that is passed to a filter as the filtered parameter. Similar filters in core do not check this either.
We could also check for an empty array, but I don't want to overengineer it.
Right, I don't think that's necessary, especially since the default isn't empty either.
There are just a few (mostly naming) changes I'd like to suggest:
- The variable could be called
$site_classes
to indicate that we're dealing with multiple classes (also makes it more obvious that it's an array). - While the site ID property is called
$userblog_id
(for legacy reasons), let's use a more appropriate name in the filter documentation (for example$site_id
). The name is independent of the actual passed parameter anyway since it's a property. - It might be useful to pass the network ID (as saved in the
$site_id
property) to the filter as well, since that is also used in the default class. Similar to the above, the docs should probably call it$network_id
. - Not very important, but I'd preferably use
implode()
instead ofjoin()
just because it is the actual and more commonly used PHP function, and since it aligns better with its counterpartexplode()
. - Last but not least, let's look at the name of the filter
ms_user_list_site_class
for a bit longer. I'm not sure whether it's appropriate/precise enough. I'm not opposed to the current name, just something to think about further.
#5
@
7 years ago
Cool, I'll make those changes.
I went with join
vs implode
after copying the body_class
function. I couldn't recall/find if there was a preference in Core. I'll switch it over since, in the end, I want the committer happy with whatever they're committing ;-)
For the length of the filter name, I'm apathetic. Tried to mimic the naming used elsewhere ms_user_list_site_actions
(noting how bad of an idea to follow MS naming schemes), but yeah, no preference on my end.
This ticket was mentioned in Slack in #core-multisite by kraft. View the logs.
7 years ago
#7
@
7 years ago
- Milestone changed from Awaiting Review to 5.0
Moving this into 5.0 to ensure consideration.
#8
@
6 years ago
- Milestone changed from 5.0 to 5.1
Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.
This ticket was mentioned in Slack in #core-multisite by kraft. View the logs.
6 years ago
#11
@
6 years ago
I am +1 to this change.
I think the patch is acceptable as is (given the current standard of how core handles this elsewhere.)
That said, here is what I wish core would enforce (in more places than this):
- The
ms_user_list_site_class
filter may not return an array, but the line of code immediately after the filter is applied trusts that it will be by usingarray_unique()
andimplode()
on it. I'd like to cast the results ofapply_filters()
as an(array)
to avoid errors. - If
$site_classes
is empty, a single space will be used in the class attribute. This is not valid. sanitize_html_class()
exists, but isn't used in so many places. Right now,$site_classes
could include invalid strings, or quotes that break out of the HTML attribute entirely. I'd like for each class in the array to be properly sanitized.esc_attr( implode( ' ', array_unique( $site_classes ) ) )
is 3 nested, inline, function calls; it feels excessive to me (when not inside a dedicated helper/convenience function.) Core does not have a standard for this, but 2 feels like the max before it gets too busy for the logical order of things to be clearly understood.
#12
@
6 years ago
Thanks @johnjamesjacoby and my apologies for overlooking the notification.
I agree on all fronts. I refreshed the patch since it didn't apply cleanly anymore and added:
- array_map with sanitize_html_class the values before output.
- moved the array_unique to have two twice-nested calls instead of the one with three.
- conditional that verified that the array was an array and was not empty
- left if outputting a span to avoid complicating the closing and to preserve the same DOM tree as some defensive BC for someone CSSing based on the element, not classes/ids.
Adds filter to the span class