#15800 closed enhancement (fixed)
Adding tabs to the "Edit Site"-pages in Network Admin
Reported by: | PuffyThePirateBoy | Owned by: | jeremyfelt |
---|---|---|---|
Milestone: | 4.6 | Priority: | normal |
Severity: | minor | Version: | 3.1 |
Component: | Networks and Sites | Keywords: | has-patch commit |
Focuses: | accessibility, administration, multisite | Cc: |
Description
There are four tabs in the Network Admin > Edit Site pages. These are statically defined as an array in the files:
wp-admin/network/site-info.php wp-admin/network/site-options.php wp-admin/network/site-themes.php wp-admin/network/site-users.php
It would be nice if there was a filter that allowed us to add more tabs to this bar. This would enable us to add more user friendly, site specific, option pages for super administrators.
Attachments (16)
Change History (62)
#4
@
14 years ago
In #15593 this enhancement was requested as part of an other issue (missing previously existing hooks). However, the patch only applies to the other issue leaving the enhancement request overlooked.
#5
@
13 years ago
- Cc bananastalktome@… added
+1 on this request. It would be great to be able to add additional tabs to the page. Any chance of this being revisited (or guidance for others who may want to submit a patch for this)?
#12
@
11 years ago
Whoever wants to pitch in have a look at this repo https://github.com/cfoellmann/WordPress-network-tabs - I will give you push access if you reply here: https://github.com/cfoellmann/WordPress-network-tabs/issues/1
#13
@
11 years ago
- Component changed from Network Admin to Networks and Sites
- Focuses administration added
#14
@
11 years ago
Found this ticket working on extending ourStream plugin to work with multisite. We need to be able to add a settings page for each site on the network so that when network activated individual site settings can be updated via the network admin menu.
I think we should stick with the scope of this ticket and while it would be nice to have a generic way to add tabs to any page my patch focuses on extending the Edit- site pages tabs and consolidating the repeated hard coded tabs html into a function that can be filtered to add the extra tab. I've also added a test case plugin that tests adding a new tab. The patch needs to be refined as it's more of a test of concept. Please provide feedback.
#16
@
11 years ago
- Focuses accessibility added
I have enhance the patch from @c3mdigital for a little bit codex and advanced ARIA.
See also on this commit https://github.com/cfoellmann/WordPress/commit/141f5b8944355c9c544da3bae7feefdaf2f461b5
#17
@
11 years ago
Usage example by @bueltge https://github.com/cfoellmann/WordPress/issues/3#issuecomment-41380678
I think we have a working solution here. I would really love the Settings API to support tabs but that is asking too much at the moment.
#18
@
10 years ago
After discussion on contributor day I have created a new, simple example plugin to explain how it works - network-tab-tester-15800.php
.
The plugin shows a basic example creating a tab named test
, slug test_tab
and creates a callback ticket15800_callback
that only runs when the tab is active to get the content.
#19
@
9 years ago
Would be very useful to have this merged as the current workarounds, such as using jQuery to inject custom tabs through admin footer, see http://wordpress.stackexchange.com/questions/103765/add-site-options-ui-in-multisite-sites-infos-page, are not nice and several plugins could use this.
This ticket was mentioned in Slack in #core-multisite by bueltge. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by bueltge. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#23
@
9 years ago
From an accessibility point of view, these "tabs" are not real tabs, not as an ARIA tabbed interface is intended, see http://www.w3.org/TR/wai-aria-practices/#tabpanel and the examples linked there. They just look like tabs but actually they're just links to other pages. Thus, they shouldn't use any ARIA roles or properties.
We discussed this a bit in the accessibility team in a ticket related to the "tabs" in the menus screen, see #33603 and considering to just wrap them in a navigation landmark.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#25
@
9 years ago
- Keywords early added
15800.2.patch does the following:
- Refreshes patch for WordPress 4.6-early
- Adds a
cap
to each tab, so they can be conditionally checked and/or skipped - Modifies each of the 4 files referenced in core tabs to include their new capabilities: (
manage_site_info
,manage_site_themes
,manage_site_users
,manage_site_settings
) - Adds more sane filters on tabs and arguments vs. HTML output
- Removes the
nav-tab-small
classification, because big is beautiful and this is the last remaining/only place in core that uses these tiny tabs - Moves the wrapper HTML into
before
andafter
arguments that can be filtered, should someone want to get fancy with this interface on their own without modifying core HTML - Adds some output buffering to encapsulate the final output of all of this processing and calculation
A few talking points after chatting with @jeremyfelt:
- What if we want to abandon these tabs later? Personally, I don't think we should, because these tabs make for a convenient and useful interface for power users in a trusted position. I can see making this fancy like the Appearance bar, but removing them seems drastic.
- How do we want to really truly allow these tabs to be filtered? I can imagine a bunch of ways, but I used Mercator as an example of adding domain mapping here. Right now, it has to use JavaScript to inject it's tab at the end, which isn't very great. I believe tabs alone could be filtered, and that someone might want to totally override this interface to suit their power-user needs, so I've included 2 filters for both of those purposes.
- Did we get the ARIA tags right? I think there's much more we can do here, but what's in the patch seems like the bare minimum and follows what WordPress is already trying to do.
- Is the output buffer absolutely necessary? No, but I like the idea of batching this output and rendering it all at once. Why do this? In the unlikely event of OOM errors, no tabs render vs partial or incomplete tabs. It also prevents jumpiness on slow connections while tabs are looped through and echo'ed. I have a tendency to do this with most loops that shouldn't communicate to the user if an interruption occurs, so I included that philosophy here. WordPress core also does this in several places already, though I consider it an unwritten rule.
- I have a somewhat selfishly urgent need to see this happen in 4.6-early, so whatever I can do to get yay/nay from interested parties and a sign-off from Jeremy, I will keep an eye on this and shepherd as needed.
- Lastly, is the function name
network_edit_site_tabs()
acceptable to everyone? All possible function names I can think of for this function are equally bad.wp_network_site_edit_tabs()
wp_network_edit_site_tabs()
maybe_wpms_site_edit_tabs_are_too_big()
et all
@
9 years ago
Add selected
argument, and pass from each core file. This makes it easier for custom pages to target and render what tab is "active" without needing to guess at globals or callback hooks.
This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.
9 years ago
#27
@
9 years ago
- Keywords needs-testing added; dev-feedback removed
I've only had time to give this a read-through, but a general +1 on where things are going. :)
A couple thoughts:
- Adding more specific capability checks makes sense. I'd probably prefer to introduce that granularity in another ticket as it will involve accounting for the current
manage_sites
capability in a back-compat way. - I do wonder if
network_edit_site_tabs
is the right place to loop through the capabilities. Some of that could instead be handled by plugins as they choose whether or not to add a tab to the filter. - What would it look like to exclude the core provided tabs from the
network_edit_site_tabs
filter? Once more granular capabilities are available, tabs could be hidden on a cap check. This would align more with the result of visiting each of these pages directly. - I'm not sure about the before/after HTML args for the tab output.
#28
@
9 years ago
- Keywords early removed
- Milestone changed from Future Release to 4.6
Bumping this into the 4.6 milestone, because the time is appropriate to figure this out. Feedback is very welcome. :)
This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.
9 years ago
#30
@
9 years ago
Discussed a bit in today's a11y bugs scrub meeting. From an accessibility and semantic point of view, these are not "tabs". They're just links that look like tabs. More like a navigation sub-menu. ARIA tablist, tab, tabpanel roles are intended for "in page" content that gets revealed / hidden when clicking on a tab. Simple example here:
http://heydonworks.com/practical_aria_examples/#tab-interface
We'd recommend to remove any ARIA added here. Also in the accessibility team we're planning to review all the different kind of "tabs" used in the admin, hopefully in an incoming feature (plugin) project.
#31
@
9 years ago
Responding to @jeremyfelt's points:
- I'm fine doing that elsewhere, but we'll need to be doubly sure we don't make any mistakes during the fork and merge of these two initiatives
- The way we've been doing this everywhere else is to make these screens objects, but we can't easily have more than 1
WP_Screen
object hanging around at any one time. I think because these tabs are put out at run time vs. precompiled oninit
or elsewhere, doing this late is the only place we currently have without inventing a new place - It looks fine until you start to have no tabs, which indicates something has gone horribly wrong. We should account for this, but it would require manually unsetting something in a forceful way, which makes me inclined to think someone doing that wants that level of control.
- I know we don't do this in very many places anymore, but considering this is duplicated logic across 4 (or more) files, it felt right to put it inside the function. Once it was in there, I could imagine core wanting to modify this in the future, and/or plugins wanting to go even further into modifying the tabbed interface into something else. Naturally, I'm doing this, so I find it pretty useful as it is.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by richardtape. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#35
@
9 years ago
Just a couple of things to note (and it's me being intentionally destructive in this testing). Apologies if this is kinda obvious.
- If you unset 'site-info' from the
$tabs
array in thenetwork_edit_site_tabs
filter, you still see the fields when visiting wp-admin/network/site-info.php?id=<id> . The same goes if you empty the array of tabs, naturally.
This might be just a 'well, cool, you can break stuff if you really want to' kinda scenario. I guess my point is it might be strange to someone to remove a tab from the list and its fields still appear.
- Removing the ARIA attributes from the patch doesn't appear to have any negative issues as far as I can tell. It's still keyboard accessible, no visual differences, nothing weird in Safari voiceover.
#36
@
9 years ago
.4.patch changes the following from .3.patch:
- Remove before/after args
- Remove capability changes for each file
- Remove
aria
attributes - Rename function away from
tabs
and towardsnav
- Adds attribute escaping to tab classes
- Adds support for default
blog_id
argument - Switched to
ob_get_clean()
as a precaution, to avoid any other arbitrary output from getting flushed
Two notes from above:
- Capability checks remain in the
network_edit_site_nav()
function. This is because the 4 coresites-*.php
files aren't loaded at the same time like plugins would be, hence there is no way to decide if they should appear without pre-registering them elsewhere (likely in another procedural function) which seems superfluous - Capability checks in this version of the patch seem silly, because all of the capabilities are the same, so it takes some imagination to consider how a plugin will need to have its capability checked
This ticket was mentioned in Slack in #core-multisite by jjj. View the logs.
9 years ago
#38
@
9 years ago
- Keywords needs-testing removed
- Owner set to jeremyfelt
- Status changed from new to reviewing
Added some before/after screenshots to highlight what changes with the removal of the nav-tab-small
class from the container.
#40
follow-up:
↓ 41
@
9 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Some feedback about [37466]:
- Comments, DocBlock descriptions and summaries, @param descriptions should all end with a period.
- We haven't used the
@link
tag to link to tickets yet and I don't think it's necessary. - The docs for the first param of
network_edit_site_nav_links
should be documented in hash notation style. It's also missing a variable name like$links
. And it should be documented that the URL is relative tonetwork_admin_url()
. - I don't see a need for the output buffer in
network_edit_site_nav()
. $classes
is a hardcoded list, so$esc_classes
seems superfluous.
#41
in reply to:
↑ 40
@
9 years ago
15800.diff makes all of these changes. Should have caught the documentation stuff.
Replying to ocean90:
Some feedback about [37466]:
- Comments, DocBlock descriptions and summaries, @param descriptions should all end with a period.
- We haven't used the
@link
tag to link to tickets yet and I don't think it's necessary.- The docs for the first param of
network_edit_site_nav_links
should be documented in hash notation style. It's also missing a variable name like$links
. And it should be documented that the URL is relative tonetwork_admin_url()
.
- I don't see a need for the output buffer in
network_edit_site_nav()
.
I think the original intention was caution. I don't think there's any problem with removing it.
$classes
is a hardcoded list, so$esc_classes
seems superfluous.
We went back and forth on this a bit. The reason it's there is because an earlier patch provided the opportunity to filter those classes. Leaving it means that we already have this in place if we start filtering in the future. It won't hurt to remove.
#44
@
9 years ago
- Keywords commit added
- Resolution worksforme deleted
- Status changed from closed to reopened
See #15593. There was already a patch from PeteMall.