Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#33313 closed defect (bug) (fixed)

Responsive list tables have no toggle row buttons when there have no row actions

Reported by: chouby's profile Chouby Owned by: helen's profile helen
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: General Keywords: has-patch commit
Focuses: ui, administration Cc:

Description

This makes all non primary columns not visible on mobile devices for list tables having no row actions. This concerns only plugins and although the fix is trivial, there is a risk to break some plugins when WP 4.3 will be released, if the authors are not aware of the issue.

Attachments (6)

33313.patch (619 bytes) - added by Chouby 10 years ago.
33313.diff (997 bytes) - added by obenland 10 years ago.
33313.2.diff (934 bytes) - added by ocean90 10 years ago.
33313.3.diff (1.7 KB) - added by helen 10 years ago.
33313-polylang-before.png (22.1 KB) - added by ocean90 10 years ago.
33313-polylang-after.png (41.3 KB) - added by ocean90 10 years ago.

Download all attachments as: .zip

Change History (28)

@Chouby
10 years ago

#1 @Chouby
10 years ago

  • Keywords has-patch added

#2 @helen
10 years ago

  • Milestone changed from Awaiting Review to 4.3

Moving in for review.

#3 follow-up: @obenland
10 years ago

What would be a good plugin to test this with?

#4 in reply to: ↑ 3 @Chouby
10 years ago

Replying to obenland:

What would be a good plugin to test this with?

My first test was with Polylang (install it, go in settings->languages, add a language and then go in the strings translation tab to get the table without row actions). However, that's no problem for me since I can update it before the release of WP 4.3.

To know if other plugins are impacted, I just looked to Rewrite rules Inspector and the bulk editor of WordPress SEO (in SEO->Tools) and noticed that the problem is worse for them as no content is rendered at all. But the proposed patch does not fix their issue.

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

#5 @obenland
10 years ago

Both Yoast SEO and Rewrite Rule Inspector are overloading the single_row() method that marks a column as primary.

@obenland
10 years ago

#6 @obenland
10 years ago

33313.diff only hides headings and cells when they have a sibling with .primary-column. That should mitigate custom implementations like Yoast SEO and Rewrite Rule Inspector use.

Since I'm not 100% familiar with how list tables work in that regard, I might be missing something.

This ticket was mentioned in Slack in #core by obenland. View the logs.


10 years ago

This ticket was mentioned in Slack in #core by obenland. View the logs.


10 years ago

#9 @helen
10 years ago

33313.patch looks good to me.

33313.diff is clever, and I like it. Would like some more eyes on testing, as I've got a little near-sightedness at this point. I also don't know that it's really a great result, though that might be okay. Yoast SEO is kind of a strange example because it's specifically small-screen-unfriendly (there's a rather large min-width value in the plugin's CSS), but a good test in that way. The problem with keeping that block layout is that it relies on the data attributes on each cell to show the column name as a label, which of course these very custom implementations don't do. It's not the end of the world, and provides users with data, just with a lot less context.

The funny thing is when in tandem with the other patch, you can end up with a toggle that doesn't do anything, though I think that's preferable to not seeing data. I think plugins that are using list tables like this are going to have to update no matter what, so I'm okay with it being less than elegant. Rewrite Rules Inspector is IMO a particularly egregious use of WP_List_Table (which is rightfully still marked private) - it is a very simple table, no row actions, no sorting, nothing.

Some shoddy screenshots below. Note that the toggle doesn't show for Yoast SEO because of that plugin's min-width value mentioned above.

Before patches:

http://s.hyhs.me/cDuI/image.png

After patches:

http://s.hyhs.me/cDlh/image.png

This ticket was mentioned in Slack in #core by obenland. View the logs.


10 years ago

@ocean90
10 years ago

#11 follow-up: @ocean90
10 years ago

33313.diff looks good, but I don't think we have to care about tables with multiple primary columns. 33313.2.diff simplifies the selectors.
I've tested this with Yoast SEO and Rewrite rules Inspector and couldn't see a difference with 33313.patch. Do we still need it?

#12 @obenland
10 years ago

  • Owner set to helen
  • Status changed from new to reviewing

#13 in reply to: ↑ 11 @Chouby
10 years ago

Replying to ocean90:

I've tested this with Yoast SEO and Rewrite rules Inspector and couldn't see a difference with 33313.patch. Do we still need it?

33313.patch was not initially designed for Yoast SEO or Rewrite rules Inspector but for tables which do not override single_row() and don't have row actions. I noticed that in Polylang when testing it against the RC. Obviously, that's no problem for me as I can patch Polylang before the WordPress release. But that could be a problem for other plugins. I was just trying to find such plugins when I noticed the second issue. I should have open another ticket. Now this one reports two issues and has two patches...

@helen
10 years ago

#14 @helen
10 years ago

  • Keywords commit added

I think we need both, and at this point it's fine to just fix both issues here. I only had one other thing, which was to only show the toggle when it's specified as a primary column so we don't get those non-functional ones. See 33313.3.diff. With a second review, I think it's good to go.

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


10 years ago

#16 follow-up: @valendesigns
10 years ago

I was unable to see any breakage in Polylang before applying the patch, so I'm unable to confirm if it is fixed with 33313.3.diff. However, both Yoast SEO & Rewrite Rules Inspector were missing table data. After the patch was applied I'm now see the missing data, though without the toggle button which is expected.

@Chouby How do I break Polylang?

#17 in reply to: ↑ 16 ; follow-up: @Chouby
10 years ago

Replying to valendesigns:

@Chouby How do I break Polylang?

The langages table is working good without the patch. To break it, you need to add a language, then go in the strings translations tab (which appears only once a language exists). The strings translations table has no row actions and is broken without the patch. I tested 33313.3.diff and that's ok for me.

#18 in reply to: ↑ 17 ; follow-up: @ocean90
10 years ago

Replying to Chouby:

The strings translations table has no row actions and is broken without the patch. I tested 33313.3.diff and that's ok for me.

Confirmed that the patch fixes the issue, see attached screenshots. (Note: Polylang is hiding the primary colum via CSS for max-width 782px.)

#19 in reply to: ↑ 18 @Chouby
10 years ago

Replying to ocean90:

(Note: Polylang is hiding the primary colum via CSS for max-width 782px.)

That won't be the case in the future as I will change the primary column to adapt the table to this new mobile view.

#20 @boonebgorges
10 years ago

Tested 33313.3.diff and it is working as advertised to fix the problems in both Polylang and wordpress-seo. Approach looks sane in both cases. +1 for commit.

#21 @helen
10 years ago

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

In 33623:

List tables: Yet more primary column fallbacks.

Some custom list tables override enough methods for the column definition fallback to never kick in, so let's ensure that toggling columns only applies when a primary column is defined in some way. We also need to show a toggle button when we can when there are no row actions.

props Chouby, obenland, ocean90.
fixes #33313.

#22 @DrewAPicture
10 years ago

In 33668:

Docs: Clarify the different return conditions in the DocBlock for WP_List_Table->handle_row_actions().

This clarification follows the introduction of primary columns in 4.3. See #33313.

Props morganestes.
Fixes #33436.

Note: See TracTickets for help on using tickets.