WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 41 hours ago

#25408 assigned enhancement

Ability to specify that a list table column is "primary"

Reported by: helen Owned by: stephdau
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch needs-testing 4.1-early
Focuses: Cc:

Description

List tables have a "primary" column of sorts; that is, the one with the row actions. We should be able to specify which column is the primary one via API, apply an appropriate class, and display the row actions in cells in that column. Currently, developers customizing the list table columns often need to re-create row actions if they do not use the post name column, or just live with the row actions appearing in a less sensible spot.

This thought stems from three things in particular:

  1. The above customization example.
  2. Old experiments (nearly exactly 2 years ago) in making list tables responsive: http://cl.ly/3F1L0N3z1M0F0Z3t0X3W in #18198.
  3. The assortment of selectors in [25595] that are not particularly robust and not easily reused by developers. We should be able to target something like .has-row-actions instead.

Attachments (8)

25408.diff (46.1 KB) - added by DaveAl 20 months ago.
25408.2.diff (49.5 KB) - added by DaveAl 20 months ago.
25408.3.diff (57.8 KB) - added by jesin 13 months ago.
Modified 25408.2.diff based on WordPress 4.0-alpha
25408.4.diff (65.5 KB) - added by stephdau 5 days ago.
25408.5.diff (59.5 KB) - added by stephdau 5 days ago.
25408.6.diff (59.9 KB) - added by stephdau 4 days ago.
25408.7.diff (60.0 KB) - added by stephdau 43 hours ago.
25408.8.diff (64.5 KB) - added by stephdau 41 hours ago.

Download all attachments as: .zip

Change History (35)

comment:1 @helen20 months ago

  • Milestone changed from Awaiting Review to 3.7

Putting in 3.7 especially to address point 3 - if not done extremely soon, we should punt it again.

comment:2 @helen20 months ago

  • Type changed from enhancement to task (blessed)

comment:3 @DaveAl20 months ago

EDIT:
Ok, I see I've misread the requirements. In any case, I've rectified that situation and attached a patch. Feedback is appreciated.

I made some mistakes in the first attachment, so please ignore it and use the second one .

In essence:
I've added two new methods to class WP_List_Table, both intended to be over-ridden, but not abstract:

  1. get_primary_column(): Takes no arguments, returns the name of the desired primary column. Hardcoded much the same way as get_columns, but just one string.
  2. handle_actions( $item, $column_name, $primary ): Takes three arguments: the item - whether post, user, comment, etc. - the current column_name, and the name of the primary column. If the column name matches the primary, then the function returns the actions to be displayed.

I've also modified the various list tables that display row actions to use this new functionality. Some have single_row over-ridden, some don't. Some override the row_actions function, others just directly created and displayed their row actions in the default primary. The latter ones are where I extracted that code to override handle_actions.

The only one I didn't touch was class-wp-links-list-table.php because I don't have the links manager plugin to test it with yet, so I left that one alone.

Here's a list of the files I modified, which will now have the class "has-row-actions" in the primary cells.
class-wp-comments-list-table.php
class-wp-list-table.php
class-wp-media-list-table.php
class-wp-ms-sites-list-table.php
class-wp-ms-themes-list-table.php
class-wp-ms-users-list-table.php
class-wp-plugins-list-table.php
class-wp-posts-list-table.php
class-wp-terms-list-table.php
class-wp-users-list-table.php

Last edited 19 months ago by DaveAl (previous) (diff)

comment:4 @johnbillion20 months ago

  • Keywords needs-patch added
  • Milestone changed from 3.7 to Future Release

@DaveAl20 months ago

@DaveAl20 months ago

comment:5 @DaveAl20 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:6 @helen13 months ago

Related: #28011, #26302.

Putting into 4.0 - this will simplify a lot of things in JS and responsive CSS, make accessibility better, and enhance developer happiness. Will review patch, unless somebody beats me to it.

comment:7 @jesin13 months ago

  • Keywords needs-refresh added; has-patch needs-testing removed

By looking at the patch and the current file I can say that the line numbers have changed a lot since the patch was uploaded. The code in the original files have changed too.

comment:8 @samuelsidler13 months ago

  • Milestone changed from Future Release to 4.0

@jesin13 months ago

Modified 25408.2.diff based on WordPress 4.0-alpha

comment:9 @jesin13 months ago

  • Keywords has-patch needs-testing added; needs-refresh removed

comment:10 @helen10 months ago

  • Type changed from task (blessed) to enhancement

A lot going on in the patch, late for 4.0 (as much as obviously I want this). Patch seems to be heading in a good direction. One thing I'm noticing is setting HTML classes - should append to the set if primary column, rather than if/else, so it's easier to follow. Places where it's echoed inline should probably be set to a variable first for clarity as well.

comment:11 @helen10 months ago

  • Keywords 4.1-early added
  • Milestone changed from 4.0 to Future Release

comment:12 @ircbot10 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:13 @jeremyfelt3 months ago

In 31674:

Reveal row actions on focus in the network users list table.

In the future, we'll re-factor this selector pile in a way that makes things cleaner. See #25408.

Props afercia.

Fixes #31003.

comment:14 @slackbot13 days ago

This ticket was mentioned in Slack in #design by helen. View the logs.

comment:15 @helen13 days ago

  • Milestone changed from Future Release to 4.3

In conjunction with #32395.

comment:16 @helen13 days ago

  • Owner set to stephdau
  • Status changed from new to assigned

comment:17 @stephdau6 days ago

@helen: Kept running with 25408.3.diff, as discussed on Slack, but it never got to dealing how a primary column is actually selected/changed. The user-level setting of primary column sounds like it should be a screen option paired with the current "Show on screen" one (lists shown/hidden columns when dealing with a list table).

This is where things get a bit complex.

  • "Show on screen" is in wp-admin/includes/screen.php
  • That's JS-enabled so that what the user selects is rendered live
  • Assuming the selection of the primary column would be a drop-down (preferred because of interactions below) or series of radio (meh, considering we list columns as checkboxes above) underneath that list of displayed columns
  • We have to select the default in initial page load
  • Then, if a user hides a column, it should be taken away from the primary columns list, live, and said list revert to its default (some special columns can't be hidden).
  • Should be added back, live, if unhidden.
  • In an ideal world, user shouldn't have to "apply" primary column change, it should be live too

What's your (and others') perspective on this?

comment:18 @stephdau6 days ago

Result of chat session with Helen regarding previous comment:

  • no UI, just programmatic column selection (ie: via filter)
  • 1 generic filter, with context passed via arguments
  • at least for "round 1"

That makes my day. Going with that.

@stephdau5 days ago

comment:19 @stephdau5 days ago

Here we go: 25408.4.diff:

  • was rebuilt from scratch based on the previous patches, brings the code in line with trunk (4.3-alpha, r32551)
  • builds upon the previous work: refactoring and based on ticket feedback
  • adds a list_table_primary_column filter in WP_List_Table::get_primary_column_name(), so developers can change the default column via the related API, with $default (returnable) and $context (screen id) as arguments (as requested).
Last edited 5 days ago by stephdau (previous) (diff)

comment:20 @stephdau5 days ago

Noticing my IDE removed some of the whitespace around parenthesis and such (when I cut and pasted)... Will fix in a future patch. Code works the same, for testing.

@stephdau5 days ago

comment:21 @stephdau5 days ago

25408.5.diff:

  • sets the plugin column as being the default again (my mistake in prev patch)
  • cleans up some whitespace issues in the previous patch (IDE auto-reformat on paste)

comment:22 follow-up: @afercia4 days ago

Tested a bit the patch, thanks very much @stephdau :) Noticed a few things worth considering:

  • small typo: for Posts/Pages the default primary column is set to "author"
  • wouldn't be better to use $this->screen->id as dynamic portion of the hook name?
  • doing the above, would we really need to pass $this->screen->id as additional variable?
  • in /network/users.php the "Sites" column needs some special treatment, it needs the "has-row-actions" CSS class (links not revealed on focus see #31003 for reference)
  • is it worth it to apply all this new stuff to Link Manager too?
  • consider to update the selector in common.js

@stephdau4 days ago

comment:23 in reply to: ↑ 22 ; follow-up: @stephdau4 days ago

Replying to afercia:

Tested a bit the patch, thanks very much @stephdau :)

Thank YOU, for the testing. :)
Added new patch: 25408.6.diff. See further info below below.

Noticed a few things worth considering:

  • small typo: for Posts/Pages the default primary column is set to "author"

Fixed in new patch, thanks. Guess that was set to my last test. :)

  • wouldn't be better to use $this->screen->id as dynamic portion of the hook name?
  • doing the above, would we really need to pass $this->screen->id as additional variable?

Personally, as a dev, I'd like both a generic with context as arg (as per prev patch), or one as you describe (context via dynamic hook name). I see value in both. Passing it as an arg is what @helen and I had decided for this round, but I'm not attached to either. See WP_List_Table::get_primary_column_name() in new patch for an example of both, from which we can decide.

  • in /network/users.php the "Sites" column needs some special treatment, it needs the "has-row-actions" CSS class (links not revealed on focus see #31003 for reference)

I can do that, but I don't understand what you mean. See https://cloudup.com/c5p3yXllT7K

  • 1st screenshot is with username as primary, edit/view show on kb focus of blog url)
  • 2nd screenshot is with blogs as primary, edit/view also show next to url, then edit link (only action for user) shows underneath the blogs list.
  • is it worth it to apply all this new stuff to Link Manager too?

Can do, so long as we're cool with the logic. I'd assume yes. Original patches I started from didn't address it, not sure why.

  • consider to update the selector in common.js

I'm not sure what that really means per se. :) Could you apply the patch, make your change, add it to the patch, and submit it, so I can see?

Thanks.

comment:24 in reply to: ↑ 23 @afercia3 days ago

Replying to stephdau:

Personally, as a dev, I'd like both a generic with context as arg (as per prev patch), or one as you describe (context via dynamic hook name). I see value in both. Passing it as an arg is what @helen and I had decided for this round, but I'm not attached to either.

I'm not attached to either too :) just a bit concerned about consistency with other similar hooks noticing that they all have a dynamic hook name.

  • in /network/users.php the "Sites" column needs some special treatment, it needs the "has-row-actions" CSS class (links not revealed on focus see #31003 for reference)

I can do that, but I don't understand what you mean.

One of the side goals of this ticket would be replacing in common.js that "terrible selectors pile" as @helen named it :) with just one CSS selector. See:
https://core.trac.wordpress.org/browser/tags/4.2/src/wp-admin/js/common.js#L481
should use as selector just ".has-row-actions". Once you do that, the edit/view links in the "Sites" column won't show on keyboard focus because that column is not the primary one and doesn't have a ".has-row-actions" class.

  • consider to update the selector in common.js

I'm not sure what that really means per se.

See above :)

@stephdau43 hours ago

comment:25 @stephdau43 hours ago

In 25408.7.diff, I've replaced if ( $primary === $column_name ) { by if ( $primary === $column_name || 'blogs' === $column_name ) { (see L170-173 in src/wp-admin/includes/class-wp-ms-users-list-table.php in patch), which will always apply has-row-actions to the blogs/sites column.

The same update also bring the patch in line with the latest trunk.

comment:26 @stephdau42 hours ago

@helen: can you give us your opinion on the filter (context as argument, or context in dynamic hook name?)? See WP_List_Table::get_primary_column_name() in patch.

Links list table: one of the comments above states "The only one I didn't touch was class-wp-links-list-table.php because I don't have the links manager plugin to test it with yet, so I left that one alone.", so now I know why it wasn't previously handled. Will add to future patch.

Last edited 42 hours ago by stephdau (previous) (diff)

@stephdau41 hours ago

comment:27 @stephdau41 hours ago

25408.8.diff adds the same logic to src/wp-admin/includes/class-wp-links-list-table.php.

Left:

  • decide how we want the filter(s) to exist in WP_List_Table::get_primary_column_name().
  • dealing with wp-admin/js/common.js if we want to do that here (I wouldn't,since it has its own ticket).
Note: See TracTickets for help on using tickets.