WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 8 days ago

#48814 new defect (bug)

Comment marked as spam displays poorly in mobile view

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

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 (8)

mobile-mark-as-spam.gif (71.7 KB) - added by jeremyfelt 2 months ago.
48814.diff (980 bytes) - added by lorenzof 2 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 2 months ago.
First patch, first mistake, I uploaded the wrong one, that's the right one
48814.2.diff (464 bytes) - added by mukesh27 2 months ago.
Patch.
48814.3.patch (459 bytes) - added by razamalik 2 months ago.
display:block change changed with display:table-cell
48814.3.2.patch (459 bytes) - added by razamalik 2 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 weeks ago.
Comment display before
48814-after.png (109.1 KB) - added by lorenzof 8 weeks ago.
Comment display after

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
2 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.


2 months ago

@lorenzof
2 months ago

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

@lorenzof
2 months ago

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

#3 @lorenzof
2 months ago

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

#4 @mukesh27
2 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
2 months ago

Patch.

#5 @lorenzof
2 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 2 months ago by lorenzof (previous) (diff)

#6 @mukesh27
2 months ago

@lorenzof Yes, it will take display property from td

@razamalik
2 months ago

display:block change changed with display:table-cell

@razamalik
2 months ago

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

#7 @lorenzof
2 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 weeks 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 weeks 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 weeks ago

Comment display before

@lorenzof
8 weeks ago

Comment display after

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


6 weeks ago

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


8 days ago

#13 @xkon
8 days 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
8 days ago

  • Keywords needs-design-feedback removed
Note: See TracTickets for help on using tickets.