WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 5 days ago

#49576 assigned defect (bug)

Menu Editor UI issues on med-large screens

Reported by: garrett-eclipse Owned by: audrasjb
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch needs-testing needs-testing-info
Focuses: ui, css, administration Cc:

Description (last modified by garrett-eclipse)

Hello,

Just reporting two UI issues with the Menu Editor;

  1. On screens not quite on the medium breakpoint but not too wide you'll find the 'Save Menu' button falls directly below the Menu Name input.

https://core.trac.wordpress.org/raw-attachment/ticket/49576/Screen%20Shot%202020-03-03%20at%202.05.04%20PM.png
*This is worst when the button is focused as the input overlaps the button.
https://core.trac.wordpress.org/raw-attachment/ticket/49576/Screen%20Shot%202020-03-03%20at%202.05.15%20PM.png

  1. On larger screens when the input is beside the button you'll find the button is slightly larger.

https://core.trac.wordpress.org/raw-attachment/ticket/49576/Screen%20Shot%202020-03-03%20at%202.05.43%20PM.png
*I've seen a few tickets flagging other parts of the UI for similar issues.

Thanks

Attachments (16)

Screen Shot 2020-03-03 at 2.05.04 PM.png (14.1 KB) - added by garrett-eclipse 11 months ago.
Create Menu directly up against Menu Name input on med-large screens.
Screen Shot 2020-03-03 at 2.05.15 PM.png (12.1 KB) - added by garrett-eclipse 11 months ago.
Button w/ focus overlapped by Menu Name input.
Screen Shot 2020-03-03 at 2.05.43 PM.png (11.3 KB) - added by garrett-eclipse 11 months ago.
Save Menu button is slightly larger in height compared to the Menu Name input
49576.patch (514 bytes) - added by man4toman 10 months ago.
49576.1.diff (558 bytes) - added by audrasjb 10 months ago.
Menus: Fixes menu editor responsive issues on med-large screens
12751439cf15f02258a8ac103389072c.gif (1.4 MB) - added by audrasjb 10 months ago.
After 59576.1.diff
002.png (7.4 KB) - added by man4toman 9 months ago.
5.2 menu name input button.png (39.2 KB) - added by afercia 9 months ago.
In WordPress 5.2 the misalignment was even more noticeable.
5.2 menu name button float drop.png (39.9 KB) - added by afercia 9 months ago.
In WordPress 5.2 the button dropped below the input as well
49576.alt.diff (542 bytes) - added by garrett-eclipse 7 months ago.
Alternate approach using margins to account for both occurrances of the issue and take variable string length from alternate locales into account.
c24325150af3cd27de8b1c43d7321ec4.gif (3.0 MB) - added by garrett-eclipse 7 months ago.
Illustrating the alternate patch. One other benefit is we no longer force Menu Name label to it's own line.
49576.3.diff (1.1 KB) - added by garrett-eclipse 3 months ago.
Refresh to avoid introducing new media query
b08c03d40f38cfc8325b595e951a4692.mp4 (4.8 MB) - added by audrasjb 3 months ago.
Patch solves the issue
49576.2.patch (1.1 KB) - added by mukesh27 3 months ago.
Updated patch.
Screen Shot 2020-11-10 at 1.53.56 PM.png (118.0 KB) - added by helen 3 months ago.
Screen Shot 2020-11-10 at 1.54.10 PM.png (94.1 KB) - added by helen 3 months ago.

Change History (52)

@garrett-eclipse
11 months ago

Create Menu directly up against Menu Name input on med-large screens.

@garrett-eclipse
11 months ago

Button w/ focus overlapped by Menu Name input.

@garrett-eclipse
11 months ago

Save Menu button is slightly larger in height compared to the Menu Name input

#1 @garrett-eclipse
11 months ago

  • Description modified (diff)

#2 @garrett-eclipse
11 months ago

#48430 is similar to item 2 I mentioned

Last edited 11 months ago by garrett-eclipse (previous) (diff)

#3 @audrasjb
10 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.5
  • Owner set to audrasjb
  • Status changed from new to assigned

#4 follow-up: @man4toman
10 months ago

It happens between 783px to 852px.

@man4toman
10 months ago

#5 @man4toman
10 months ago

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

#6 in reply to: ↑ 4 @JavierCasares
10 months ago

Replying to man4toman:

It happens between 783px to 852px.

It happens to me at 783-852, and 961-976.

The patch fixed the first one, but not the second one... :(

@audrasjb
10 months ago

Menus: Fixes menu editor responsive issues on med-large screens

#7 @audrasjb
10 months ago

  • Keywords needs-testing removed

Thanks for your patch @man4toman,

I updated the patch, in 49576.1.diff:

  • change media query’s values to (min-width: 783px) and (max-width: 870px) to handle more use cases
  • some coding standards fixes

I think it's good to go now, thanks!
Adding a screenshot and commit keyword.

Cheers,
Jb

@audrasjb
10 months ago

After 59576.1.diff

#8 @audrasjb
10 months ago

  • Keywords commit added

#9 @man4toman
10 months ago

@JavierCasares Yes you right, I missed second one.

Thanks @audrasjb, updated patch works fine.

#10 @garrett-eclipse
9 months ago

Thanks for the patches @man4toman & @audrasjb and for testing @JavierCasares. Testing the latest patch it's working nicely for myself on both the new menu and edit menu screens.

@audrasjb on my second item in the ticket should I break that into a unique ticket, should we handle here or is there another ui ticket out there that's handling that discrepancy between field and button heights.

On larger screens when the input is beside the button you'll find the button is slightly larger.

https://core.trac.wordpress.org/raw-attachment/ticket/49576/Screen%20Shot%202020-03-03%20at%202.05.43%20PM.png

*I've seen a few tickets flagging other parts of the UI for similar issues.

Appreciated

#11 @man4toman
9 months ago

We can remove button-large class name from Create/Save Menu, it fixes the heights issues.
But another thing will appears, the heights of two Create/Save Menu button will not be same.

@man4toman
9 months ago

#12 @audrasjb
9 months ago

Hey,

I think the button height issue should probably be handled separately, or in #48531, as it's something that is more visible since WP 5.3 CSS admin changes.

#13 @garrett-eclipse
9 months ago

Thanks for taking a look @man4toman, so this ticket can move forward I moved that secondary issue to #48531 as @audrasjb it seems like a better home.
Let's get that first issue committed here and can look into the other in that more appropriate ticket.

#14 @afercia
9 months ago

Thanks @garrett-eclipse. Just to clarify, the vertical alignment issue wasn't introduced in WordPress 5.3.

For issues related to CSS it would be nice to always compare with previous versions to verify what is the original cause of the problem. As mentioned by @man4toman, the button has a button-large class that makes its height 32 pixels. It's taller than the input field. I don't see a good reason why this button should be large so I'd just remove the button-large class.

In WordPress 5.2 this was even more evident, see screenshot below. On 5.2 I can reproduce also the button dropping down the input and the focus style issue (though less visible).

I'd say 5.3 slightly improved the alignment. Regarding the general issue of vertical alignment between various form controls, it's a long-standing problem in the admin. Often, the root cause is that the CSS vertical-align property is used inconsistently across the admin on a case-by-case basis.

In the long term, case-by-case adjustments are just bad because they introduce a huge amount of inconsistencies that are really hard to fix. For some CSS sanity, exceptions and special cases should be removed. This issue was also noted in https://core.trac.wordpress.org/ticket/48420#comment:20

I'm at a point where a decision needs to be made. To make all form controls have by default a good vertical alignment (especially with visible labels or other text close to them) there are two possible options I can think of:

  • use flexbox: however, this would require a wrapper element as a flex container thus a considerable amount of markup output should be changed, which is not ideal
  • use CSS properties of the inline formatting context (inline-level boxes) like vertical-align

I'd lean towards using vertical-align: middle for all form controls as it's able to align elements and text of different heights. However, this would imply that form controls shouldn't use top or bottom margins.

A decision wasn't made and this issue still need to be addressed across the whole admin.

@afercia
9 months ago

In WordPress 5.2 the misalignment was even more noticeable.

@afercia
9 months ago

In WordPress 5.2 the button dropped below the input as well

@garrett-eclipse
7 months ago

Alternate approach using margins to account for both occurrances of the issue and take variable string length from alternate locales into account.

#15 @garrett-eclipse
7 months ago

  • Focuses administration added
  • Keywords needs-testing added; commit removed

Thanks @afercia I appreciate the overview and whole-heartedly agree we should avoid case-by-case adjustments here. Let's leave the button size and alignment items to the associated tickets (#48531 & #48420) where a proper decision and action can be taken for those inconsistencies. For this specific bug the patch @audrasjb provided (49576.1.diff) still applies properly but only resolved the issue for one breakpoint and didn't account for it breaking between x and y.

I looked to expand the media query but also found string length due to locale comes into account too and almost ended up with the query applying to most desktop. Attempting just reducing the input width worked without the media query but didn't work when testing with longer string locales like Armenian.

In 49576.alt.diff I found an alternate solution using margins to give the input and button their own space regardless of their proximity or the length of the locale string.

This approach seems to work nicely and would love input from the team. If we're all on board let's apply this fix for this specific issue.

@garrett-eclipse
7 months ago

Illustrating the alternate patch. One other benefit is we no longer force Menu Name label to it's own line.

#16 follow-up: @afercia
7 months ago

@garrett-eclipse looking at the alternative approach, looks like a good approach. Couple considerations:

  • I wouldn't add a new breakpoint min-width: 783px, that would be unprecedented in core. I'd tend to think we should try to reuse existing breakpoints. Would it be possible to reverse the approach? As in: add those rules by default and reset them when less than 782px.
  • I see you're testing when no menu exists. When a menu with items nested by 2-3 levels is set, this page responsive behavior is pretty different and basically breaks for viewport widths between about 950 / 782 pixels. This is a pre-existing issue unrelated to this patch. Just to note that this page has more serious, long-standing, layout issues that would require a major refactoring. In a separate ticket :)

#17 @audrasjb
7 months ago

  • Milestone changed from 5.5 to 5.6

This ticket still needs some work.
Given we are very close to be ready, let's move this one to 5.6 instead of Future release.

Thanks @garrett-eclipse for your work.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


5 months ago

#19 @kburgoine
5 months ago

  • Keywords needs-design added; has-patch removed

This was discussed during the #core-css triage meeting and the question was asked if we needed to keep both Create Menu buttons? Or if we could remove one?

See the transcript here: https://wordpress.slack.com/archives/CQ7V4966Q/p1598560491097900

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


5 months ago

#21 in reply to: ↑ description @tray
5 months ago

  • Keywords needs-design removed

design not needed. discussed triage 9/7/20

Last edited 5 months ago by tray (previous) (diff)

#22 @tray
5 months ago

design not needed. discussed triage 9/7/20 (sorry duplicate comment)

Last edited 5 months ago by tray (previous) (diff)

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


4 months ago

#24 @garrett-eclipse
4 months ago

  • Keywords has-patch needs-refresh added

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


3 months ago

@garrett-eclipse
3 months ago

Refresh to avoid introducing new media query

#26 in reply to: ↑ 16 @garrett-eclipse
3 months ago

  • Keywords needs-refresh removed

Replying to afercia:

@garrett-eclipse looking at the alternative approach, looks like a good approach. Couple considerations:

  • I wouldn't add a new breakpoint min-width: 783px, that would be unprecedented in core. I'd tend to think we should try to reuse existing breakpoints. Would it be possible to reverse the approach? As in: add those rules by default and reset them when less than 782px.
  • I see you're testing when no menu exists. When a menu with items nested by 2-3 levels is set, this page responsive behavior is pretty different and basically breaks for viewport widths between about 950 / 782 pixels. This is a pre-existing issue unrelated to this patch. Just to note that this page has more serious, long-standing, layout issues that would require a major refactoring. In a separate ticket :)

Thanks @afercia I appreciate the feedback. I've updated the implementation to avoid a new media query in 49576.3.diff. I think it just needs some testing and is good to go.

Through the review process there was mention of the dual buttons on many occasion as well as the layout issues you mentioned. These I feel are out of the scope of this ticket and more an enhancements so don't want them to derail this fix. As such I opened #51631 to address those items in a future release as an enhancement.

@afercia / @audrasjb could you retest and see if we can get this in before Beta 2.

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


3 months ago

@audrasjb
3 months ago

Patch solves the issue

#28 @audrasjb
3 months ago

  • Keywords commit added; needs-testing removed

Thanks @garrett-eclipse, you rock 🙌
The patch is good to go on my side.

#29 @mukesh27
3 months ago

  • Keywords needs-refresh added

@garrett-eclipse In patch 49576.3.diff can you please marge two margin property in single? like below

#save_menu_header {
        margin: 0;
	margin-bottom: 4px;
}

Replace to

#save_menu_header {
	margin: 0 0 4px;
}

@mukesh27
3 months ago

Updated patch.

#30 @mukesh27
3 months ago

  • Keywords needs-refresh removed

#31 @garrett-eclipse
3 months ago

Nice catch @mukesh27, thank you. And Thanks @audrasjb for testing. This is looking good to go.

#32 follow-up: @helen
3 months ago

  • Keywords commit removed
  • Milestone changed from 5.6 to 5.7

I don't know if maybe I'm not testing this correctly or if I'm managing to have a cache issue despite trying this in multiple browsers, but the latest patch doesn't seem to fix this for me. I think this is a fairly minor bug for the moment so I'm going to punt from 5.6 so we can focus on other things now that we're in late beta.

#33 in reply to: ↑ 32 @garrett-eclipse
2 months ago

Replying to helen:

I don't know if maybe I'm not testing this correctly or if I'm managing to have a cache issue despite trying this in multiple browsers, but the latest patch doesn't seem to fix this for me. I think this is a fairly minor bug for the moment so I'm going to punt from 5.6 so we can focus on other things now that we're in late beta.

Thanks for testing @helen, your screenshots illustrate the improved behaviour here as it was resolving this issue of interference between the button focus and input;
https://core.trac.wordpress.org/attachment/ticket/49576/Screen%20Shot%202020-03-03%20at%202.05.15%20PM.png

No worries on punting to 5.7, we'll visit the correlated enhancement ticket #51631 hopefully in conjunction.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


2 months ago

#35 @sabernhardt
2 months ago

  • Keywords needs-testing added

#36 @hellofromTonya
5 days ago

  • Keywords needs-testing-info added

Scheduling this ticket for Testing Scrub, but need more information for the testers to QA it:

  • What are the steps to reproduce the problem?
  • Are there any testing dependencies such as a plugin or script?
  • What is the expected behavior after applying the patch?
Note: See TracTickets for help on using tickets.