#33313 closed defect (bug) (fixed)
Responsive list tables have no toggle row buttons when there have no row actions
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (28)
#4
in reply to:
↑ 3
@
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.
#5
@
10 years ago
Both Yoast SEO and Rewrite Rule Inspector are overloading the single_row()
method that marks a column as primary.
#6
@
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
@
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:
After patches:
This ticket was mentioned in Slack in #core by obenland. View the logs.
10 years ago
#11
follow-up:
↓ 13
@
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?
#13
in reply to:
↑ 11
@
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...
#14
@
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:
↓ 17
@
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:
↓ 18
@
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:
↓ 19
@
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
@
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
@
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.
Moving in for review.