WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

#47173 closed defect (bug) (fixed)

Nav Menus: Delete Menu action link is misaligned in responsive view

Reported by: ianbelanger Owned by: audrasjb
Milestone: 5.2.1 Priority: normal
Severity: normal Version: 4.9
Component: Menus Keywords: has-patch has-screenshots fixed-major
Focuses: Cc:

Description (last modified by ianbelanger)

When adjusting line heights to unitless values, I noticed that the Delete Menu action link on the Menus page is misaligned when the screen is under 782px. Screenshot and patch coming

Attachments (8)

47173-delete-menu-action-misaligned-responsive-view.PNG (10.8 KB) - added by ianbelanger 4 months ago.
Before applying patch
47173.diff (366 bytes) - added by ianbelanger 4 months ago.
adds padding-top: 1em; to delete menu action
47173-delete-menu-action-misaligned-responsive-view-after.PNG (10.8 KB) - added by ianbelanger 4 months ago.
After applying patch
Remove top padding from publish button.png (32.6 KB) - added by mukesh27 4 months ago.
47173.1.diff (417 bytes) - added by ianbelanger 4 months ago.
Updates patch to remove publishing action top padding
47173.2.diff (422 bytes) - added by ianbelanger 4 months ago.
Updates patch to only target top publishing action with the top padding
47173-after-patch.PNG (27.2 KB) - added by ianbelanger 4 months ago.
After applying 47173.2.diff
4.8-trunk.png (16.0 KB) - added by desrosj 4 months ago.

Download all attachments as: .zip

Change History (24)

@ianbelanger
4 months ago

adds padding-top: 1em; to delete menu action

#1 @ianbelanger
4 months ago

  • Keywords has-patch has-screenshots needs-testing added

#2 @audrasjb
4 months ago

  • Milestone changed from Awaiting Review to 5.2.1
  • Owner set to audrasjb
  • Status changed from new to reviewing

#3 @audrasjb
4 months ago

Hi,
Thanks for the patch, I'll get this one reviewed soon :-)

#4 @audrasjb
4 months ago

  • Keywords needs-testing removed
  • Status changed from reviewing to accepted

I tested 47173.diff, it applies well and it fixed the issue on my side.
This should be fixed in 5.2.1 since it's a regression coming from 5.2.

Thanks @ianbelanger :-)

#5 @ianbelanger
4 months ago

Thanks @audrasjb, although I don't think that this issue was introduced in 5.2. I am seeing it in 5.1.1 as well, but it wouldn't hurt to get it in 5.2.1. Thanks for the quick review!

#6 @ianbelanger
4 months ago

  • Description modified (diff)

#7 @mukesh27
4 months ago

@ianbelanger instead of adding top padding can we remove top padding from publish button? check attached screenshot after remove padding from publish button.

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/css/nav-menus.css#L838

.nav-menus-php .major-publishing-actions .publishing-action {
	padding-top: 1em;
}

#8 @garrett-eclipse
4 months ago

  • Keywords needs-refresh added

Thanks @ianbelanger reviewing the patch and screenshot @mukesh27 provided I agree removing the top padding provides a nicer presentation as it gives the action a vertical centering in the space with equal padding above/below and to the right of the action. With that in mind going to add a refresh.

@ianbelanger
4 months ago

Updates patch to remove publishing action top padding

#9 @ianbelanger
4 months ago

  • Keywords needs-refresh removed

#10 @afercia
4 months ago

  • Keywords needs-refresh added
  • Version set to 4.9

I think this was introduced in WordPress 4.9 see [41197] / #41499.

The top padding should stay for the "Save Menu" at the top of the page but shouldn't target also the "Save Menu" at the bottom.

#11 @desrosj
4 months ago

  • Keywords needs-design-feedback added
  • Milestone changed from 5.2.1 to 5.2.2

This still needs a refresh, and I'd love for it to get some design attention. Going to move it to 5.2.2.

@ianbelanger
4 months ago

Updates patch to only target top publishing action with the top padding

#12 @ianbelanger
4 months ago

  • Keywords commit added; needs-refresh needs-design-feedback removed
  • Milestone changed from 5.2.2 to 5.2.1

I was actually working on a fix for this when you changed the milestone @desrosj. Just uploaded the patch to fix the issue. Not sure that we need design-feedback since it is a regression from 4.9, but if someone from design wants to weigh-in that would be great. Going to move back into 5.2.1 milestone and mark for commit.

@ianbelanger
4 months ago

After applying 47173.2.diff

@desrosj
4 months ago

#13 @desrosj
4 months ago

The left of my screenshot is 4.8 and the right is with 47173.2.diff. Since this is a regression, let's get this in.

#14 @desrosj
4 months ago

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

In 45324:

Menus: Fix alignment of menu actions on mobile.

In [41197], changes were made to improve the responsive view for the Menu Name field and Save buttons. This caused the Delete Menu button to be misaligned.

Props ianbelanger, audrasjb, afercia, mukesh27.
Fixes #47173.

#15 @desrosj
4 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#16 @desrosj
4 months ago

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

In 45325:

Menus: Fix alignment of menu actions on mobile.

In [41197], changes were made to improve the responsive view for the Menu Name field and Save buttons. This caused the Delete Menu button to be misaligned.

Merges [45324] to the 5.2 branch.

Props ianbelanger, audrasjb, afercia, mukesh27.
Fixes #47173.

Note: See TracTickets for help on using tickets.