Make WordPress Core

Opened 14 years ago

Closed 8 years ago

Last modified 8 years ago

#15800 closed enhancement (fixed)

Adding tabs to the "Edit Site"-pages in Network Admin

Reported by: puffythepirateboy's profile PuffyThePirateBoy Owned by: jeremyfelt's profile 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)

15800.patch (6.1 KB) - added by c3mdigital 11 years ago.
tab-tester.php (1.5 KB) - added by c3mdigital 11 years ago.
Test case plugin add new tab plugin
0001-Enhance-tabs-15800.patch (7.3 KB) - added by Bueltge 11 years ago.
network-tab-tester-15800.php (1.5 KB) - added by Bueltge 10 years ago.
Tab tester, readable simple source
15800.png (21.1 KB) - added by Bueltge 10 years ago.
Screenshot of tab tester example plugin
Screen Shot 2016-03-06 at 1.09.34 PM.png (72.7 KB) - added by johnjamesjacoby 9 years ago.
Bigger tabs
15800.2.patch (10.0 KB) - added by johnjamesjacoby 9 years ago.
Refresh, and refactor the original patch a bit
15800.3.patch (10.2 KB) - added by johnjamesjacoby 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.
Screen Shot 2016-04-18 at 3.15.12 PM.png (54.6 KB) - added by johnjamesjacoby 9 years ago.
Tabs removed; Tab added
15800.4.patch (8.9 KB) - added by johnjamesjacoby 8 years ago.
15800.5.patch (8.5 KB) - added by johnjamesjacoby 8 years ago.
standard-before-patch.png (34.0 KB) - added by jeremyfelt 8 years ago.
Standard view, before 15800.5.patch
mobile-before-patch.png (31.9 KB) - added by jeremyfelt 8 years ago.
Mobile view before 15800.5.patch
standard-after-patch.png (36.4 KB) - added by jeremyfelt 8 years ago.
Standard view, after 15800.5.patch
mobile-after-patch.png (28.9 KB) - added by jeremyfelt 8 years ago.
Mobile view after 15800.5.patch
15800.diff (2.6 KB) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (62)

#1 @ocean90
14 years ago

  • Version set to 3.1

See #15593. There was already a patch from PeteMall.

#2 @westi
14 years ago

This is a great idea for the future.

#3 @westi
14 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @PuffyThePirateBoy
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 @bananastalktome
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)?

#6 @cfoellmann
11 years ago

I could really use such a hook.
Maybe I have some time to look into it soon.

#7 @cfoellmann
11 years ago

  • Cc foellmann@… added

#8 @gogger
11 years ago

  • Cc gogger added

#9 @Bueltge
11 years ago

  • Cc frank@… added

#10 @toscho
11 years ago

  • Cc info@… added

#11 @cfoellmann
11 years ago

Any idea for a starting point? Crude idea how we should tackle this?

#12 @cfoellmann
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 @nacin
11 years ago

  • Component changed from Network Admin to Networks and Sites
  • Focuses administration added

#14 @c3mdigital
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.

@c3mdigital
11 years ago

@c3mdigital
11 years ago

Test case plugin add new tab plugin

#15 @rhurling
11 years ago

  • Keywords has-patch dev-feedback added

#16 @Bueltge
11 years ago

  • Focuses accessibility added

I have enhance the patch from @c3mdigital for a little bit codex and advanced ARIA.

Version 0, edited 11 years ago by Bueltge (next)

#17 @cfoellmann
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.

Last edited 11 years ago by cfoellmann (previous) (diff)

@Bueltge
10 years ago

Tab tester, readable simple source

@Bueltge
10 years ago

Screenshot of tab tester example plugin

#18 @Bueltge
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.

Screenshot of tab tester example plugin

Last edited 10 years ago by Bueltge (previous) (diff)

#19 @basteln3rk
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 @afercia
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 @johnjamesjacoby
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 and after 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
  • I decided not to include the callback idea previously explored in this ticket. I think it's a good idea, but out of scope for the general improvement of encapsulating these tabs into a central DRY approach. Once we get that done, we should probably make new tickets for future enhancements.

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
Last edited 9 years ago by johnjamesjacoby (previous) (diff)

@johnjamesjacoby
9 years ago

Refresh, and refactor the original patch a bit

@johnjamesjacoby
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 @jeremyfelt
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 @jeremyfelt
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 @afercia
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 @johnjamesjacoby
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 on init 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.
Last edited 9 years ago by johnjamesjacoby (previous) (diff)

@johnjamesjacoby
9 years ago

Tabs removed; Tab added

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 @iamfriendly
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.

  1. If you unset 'site-info' from the $tabs array in the network_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.

  1. 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 @johnjamesjacoby
8 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 towards nav
  • 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 core sites-*.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.


8 years ago

@jeremyfelt
8 years ago

Standard view, before 15800.5.patch

@jeremyfelt
8 years ago

Mobile view before 15800.5.patch

@jeremyfelt
8 years ago

Standard view, after 15800.5.patch

@jeremyfelt
8 years ago

Mobile view after 15800.5.patch

#38 @jeremyfelt
8 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.

#39 @jeremyfelt
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37466:

Multisite: Filter the links displayed on "Edit Site" views

Introduce the network_edit_site_nav function, which DRYs up the code used to display a common set of links at the top of "Edit Site" views.

Introduce the network_edit_site_nav_links filter, which allows plugins to modify the list of links displayed at the top of Edit Site views as a "tabbed" interface.

Props johnjamesjacoby, c3mdigital, Bueltge.
Fixes #15800.

#40 follow-up: @ocean90
8 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 to network_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.

@jeremyfelt
8 years ago

#41 in reply to: ↑ 40 @jeremyfelt
8 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 to network_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.

#42 @DrewAPicture
8 years ago

  • Keywords has-patch added; needs-patch removed

#43 @ajv9540
8 years ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

#44 @ocean90
8 years ago

  • Keywords commit added
  • Resolution worksforme deleted
  • Status changed from closed to reopened

#45 @jeremyfelt
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37629:

Multisite: Remove superfluous code from network_edit_site_nav()

  • Remove output buffering.
  • Remove esc_attr() when outputting hard coded class names.
  • Update documentation.

Fixes #15800.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.