WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 6 months ago

Last modified 3 months ago

#48814 closed defect (bug) (fixed)

Comment marked as spam displays poorly in mobile view

Reported by: jeremyfelt Owned by: adamsilverstein
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Comments Keywords: good-first-bug has-patch has-screenshots commit
Focuses: ui, css Cc:

Description

When a comment is marked as spam from wp-admin/edit-comments.php in a mobile view, the record of the comment with the option to undo is displayed in too small of an area rather than across the comment list table.

I believe this occurs because display of the .undo td is set to block in the list table CSS when it should be the default of table-cell.

Describing this is weird, so I'm attaching a GIF. :)

Attachments (9)

mobile-mark-as-spam.gif (71.7 KB) - added by jeremyfelt 8 months ago.
48814.diff (980 bytes) - added by lorenzof 8 months ago.
Hi, I created a patch for the ticket, it's my first patch so please check if everything is ok.
48814.1.diff (509 bytes) - added by lorenzof 8 months ago.
First patch, first mistake, I uploaded the wrong one, that's the right one
48814.2.diff (464 bytes) - added by mukesh27 8 months ago.
Patch.
48814.3.patch (459 bytes) - added by razamalik 8 months ago.
display:block change changed with display:table-cell
48814.3.2.patch (459 bytes) - added by razamalik 8 months ago.
display:block change changed with display:table-cell and i have tested in different browsers working fine! @lorenzof
48814-before.png (98.0 KB) - added by lorenzof 8 months ago.
Comment display before
48814-after.png (109.1 KB) - added by lorenzof 8 months ago.
Comment display after
jjj-sc-2020-05-19 at 15.29.43@2x.png (130.9 KB) - added by johnjamesjacoby 3 months ago.
Mark as Spam on a Small Screen, then make your browser width wide again

Download all attachments as: .zip

Change History (30)

#1 @SergeyBiryukov
8 months ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

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


8 months ago

@lorenzof
8 months ago

Hi, I created a patch for the ticket, it's my first patch so please check if everything is ok.

@lorenzof
8 months ago

First patch, first mistake, I uploaded the wrong one, that's the right one

#3 @lorenzof
8 months ago

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

#4 @mukesh27
8 months ago

@lorenzof @jeremyfelt If we remove the display property from CSS .wp-list-table tr:not(.inline-edit-row):not(.no-items) td:not(.check-column) then it will fix the issue.

@mukesh27
8 months ago

Patch.

#5 @lorenzof
8 months ago

Thanks for the update @mukesh27

You removed the display property so that it inherits the table-cell value from td, and in that case there's no need to repeat the display: table-cell as I did my patch, right?

Last edited 8 months ago by lorenzof (previous) (diff)

#6 @mukesh27
8 months ago

@lorenzof Yes, it will take display property from td

@razamalik
8 months ago

display:block change changed with display:table-cell

@razamalik
8 months ago

display:block change changed with display:table-cell and i have tested in different browsers working fine! @lorenzof

#7 @lorenzof
8 months ago

@razamalik The .2 version of the patch submitted by @mukesh27 was fine and it's the best one since it's redundant to only change the value, so why you added a new version creating a patch that's the same one I submitted as .1 version, reverting his changes? .2 is fine, that's the one that need testing

#8 follow-up: @jeremyfelt
8 months ago

  • Keywords needs-screenshots added

Thanks for the patches @lorenzof, @mukesh27, and @razamalik. If I follow correctly, it sounds like 48814.2.diff is the proposed solution—remove the display property for mobile?

It would also be helpful to have screenshots showing the change.

#9 in reply to: ↑ 8 @lorenzof
8 months ago

Replying to jeremyfelt:

Thanks for the patches @lorenzof, @mukesh27, and @razamalik. If I follow correctly, it sounds like 48814.2.diff is the proposed solution—remove the display property for mobile?

It would also be helpful to have screenshots showing the change.

Yes, that's the right one. I appreciate the help from @razamalik who probably hasn't seen my patch but the right one it's the patch from @mukesh27. Less code, same result considering that the display: table-cell is inherited. I'll attach two screenshots from latest trunk.

@lorenzof
8 months ago

Comment display before

@lorenzof
8 months ago

Comment display after

#10 @jeremyfelt
8 months ago

  • Keywords has-screenshots needs-design-feedback added; needs-screenshots removed
  • Milestone changed from Future Release to 5.4

Excellent, thanks @lorenzof! I'm going to add a design feedback tag here, but I think things are looking good for 5.4.

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


8 months ago

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


7 months ago

#13 @xkon
7 months ago

After today's #design triage, I'm removing the needs-design-feedback as everything looks good :).

I was having some issues applying the patch so it might need a refresh, although it's a small change not sure if a committer can handle it directly :).

#14 @xkon
7 months ago

  • Keywords needs-design-feedback removed

#15 @audrasjb
6 months ago

Hi,

This small change should be refreshed and committed very soon, we are currently punting the remaining tickets in milestone 5.4 to 5.5.

Thanks!
Jb

#16 @adamsilverstein
6 months ago

https://core.trac.wordpress.org/attachment/ticket/48814/48814.2.diff looks reasonable - do we need to adjust list-tables-rtl.css as well as in earlier versions of the patch?

This ticket was mentioned in PR #162 on WordPress/wordpress-develop by adamsilverstein.


6 months ago

#18 @adamsilverstein
6 months ago

  • Keywords commit added; needs-testing removed
  • Owner set to adamsilverstein
  • Status changed from new to assigned

I verified the rtl change is made automatically by our build script, so no need to include it in the patch. Testing in RTL (with the rtl tester plugin) the patch works as expected:

https://p10.f2.n0.cdn.getcloudapp.com/items/NQuZd5xG/Image+2020-02-21+at+9.25.07+AM.png

#19 @adamsilverstein
6 months ago

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

In 47340:

Comments: improve styling on mobile for comments marked as spam.

Correct an issue where comments marked as spam in a mobile view displayed incorrectly, filling a narrow and very tall column with the notice that a comment was marked as spam.

Props jeremyfelt, lorenzof, mukesh27, razamalik, xkon.
Fixes #48814.

#20 @johnjamesjacoby
3 months ago

Apologies, but unfortunately this minor change has caused a somewhat major regression in one of my plugins.

I can fix this for my users, but I am concerned about unreported breakage in other plugins due to the relatively reduced footprint of mobile users attempting to manage their content within WordPress Admin.


The display: block; that was removed in r47340 was necessary to override the default display: table-cell styling. Screenshot incoming to show why it is needed.

table-cell and width: auto !important will not allow for the targeted tds to occupy the full-width of the parent element.

It is also why the following rule...

.wp-list-table .is-expanded td:not(.hidden)

...still requires display: block !important;. Otherwise, it would also unintentionally risk not being full-width.


In the following rule, WordPress avoids applying styling to certain tr elements (including the dynamic .inline-edit-row one) using :not() - like:

.wp-list-table tr:not(.inline-edit-row):not(.no-items) td:not(.check-column)

I believe the non-breaking change here would have been to leave the display: block; alone, and then to add the unspam and untrash classes to the group of row classes not to target, like:

.wp-list-table tr:not(.inline-edit-row):not(.no-items):not(.unspam):not(.untrash) td:not(.check-column)

Will leave this up to the contributors here to decide how best to proceed. Sorry again, and thanks for looking & understanding. 💙

Last edited 3 months ago by johnjamesjacoby (previous) (diff)

@johnjamesjacoby
3 months ago

Mark as Spam on a Small Screen, then make your browser width wide again

#21 @johnjamesjacoby
3 months ago

Mitigated downstream in Sugar Calendar in this pull request.

Note: See TracTickets for help on using tickets.