WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 8 hours ago

#51631 accepted enhancement

Refactor Layout of the Menu screens to avoid dual-buttons and responsive issues.

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

Description

This ticket is split from #49576 to further investigate improvements on the responsive layout of the Menu screens, and hopefully avoid dual primary buttons in close proximity.

Quoting specific discussion points below.

comment#16 by @afercia:

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 :)

slackComment by @helen:

Why are there two Create Menu buttons next to each other like that?

slackDiscussion from @kburgoine

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?

Attachments (13)

Capture d’écran 2020-11-06 à 16.43.02.png (108.8 KB) - added by audrasjb 3 months ago.
Two buttons issue / 1
Capture d’écran 2020-11-06 à 16.42.47.png (170.8 KB) - added by audrasjb 3 months ago.
Two buttons issue / 2
51631.1.diff (573 bytes) - added by maxpertici 6 weeks ago.
sticky-on-header
51631.1-sticky-on-header.png (149.1 KB) - added by maxpertici 6 weeks ago.
sticky-on-header
51631.2.diff (558 bytes) - added by maxpertici 6 weeks ago.
sticky-on-footer
51631.2-sticky-on-footer.png (118.6 KB) - added by maxpertici 6 weeks ago.
sticky-on-footer
51631-large-screens.gif (3.5 MB) - added by audrasjb 2 days ago.
51631.2.diff – on large screens
51631-med-screens.gif (3.5 MB) - added by audrasjb 2 days ago.
51631.2.diff – on medium screens
51631-small-screens.gif (1.3 MB) - added by audrasjb 2 days ago.
51631.2.diff – on small screens
expanded-menu-item-overlaps.png (13.7 KB) - added by sabernhardt 13 hours ago.
z-index issue with expanded menu item
menu-sticky-covered-input.png (18.6 KB) - added by sabernhardt 13 hours ago.
footer covers checkboxes (and their label) even when in focus
51631.3.diff (825 bytes) - added by maxpertici 10 hours ago.
51631.3.png (164.4 KB) - added by maxpertici 10 hours ago.

Change History (28)

#1 @kburgoine
3 months ago

During last weeks #core-css triage, we discussed this issue briefly again in relation to #49576 and there was a question raised around whether we could do something purely with CSS that could fix this

Question for CSS gurus, is there a css way to stick the bottom one at the bottom of the container until the container bottom is out of view then have it at bottom of view. Or is JS the only route for that stuff still

https://wordpress.slack.com/archives/CQ7V4966Q/p1603398632334500

I've spent some time looking into this and can't find a way to do this purely with CSS (maybe someone else can? if so please share, I would love to know!). CSS doesn't have a way to calculate the height of a given container. You could use 100vh for the height of the viewport but, to know if the container displays entirely on the screen you would also have to know the height of the header area and the height of the container itself.

For example, if you knew the exact height of the page header area (in this case, shows the edit menu's and menu location tabs as well as the select box to choose a menu to edit) was 35vh and you also knew that the specific container for the menu items was 75vh then you could use something along the lines of a SASS function to add those two heights together and if the total is greater than 100, set a specific behaviour.

However, we can't determine any of those values because the header area height will change responsively and could have a larger height on smaller screens, where items have to stack. The size of the container also depends on the number of menu items added to it. We would need Javascript to determine those values for us, and once you start using javascript there is probably a much more elegant solution.

We could add some javascript here, it wouldn't be too difficult, but is there then a question of over-engineering?

In my mind, the simplest solution is to remove the bottom button when it is a create menu button and keep it there when it is a save menu button.

It's not a perfect solution because when you have first created your menu until you start adding menu items there are still two buttons in close proximity, but as you add menu items it does at least make sense as to why the bottom button is also there. Thoughts?

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


3 months ago

@audrasjb
3 months ago

Two buttons issue / 1

@audrasjb
3 months ago

Two buttons issue / 2

#3 @audrasjb
3 months ago

  • Keywords needs-patch has-screenshots added
  • Milestone changed from Awaiting Review to 5.7

As per today's accessibility bug scrub, the second "Create menu" button doesn't looks necessary and can be removed.
But the redundant "Save menu" button can be useful when there is a lot of menu items.

Moving to milestone 5.7.

#4 @garrett-eclipse
3 months ago

  • Keywords needs-design added

IMO I'd suggest keeping the button at the bottom as it provides a more logical form flow (actions after inputs). For the create screen definitely drop the top one.

For the Save screen am wondering if we could use JS to fix the bottom actions to always be visible but keep them in their container if you scroll past. This would allow us to also be consistent between the two views, removing the top button.

If JS isn't an option maybe we suppress the top button when there's less than X menu items. So suppressed on create and on save when say only 3 menu items available.

Thoughts? Added needs-design to get input from design on the approach.

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


2 months ago

@maxpertici
6 weeks ago

sticky-on-header

@maxpertici
6 weeks ago

sticky-on-header

@maxpertici
6 weeks ago

sticky-on-footer

@maxpertici
6 weeks ago

sticky-on-footer

#6 follow-up: @maxpertici
6 weeks ago

Hello,

From my point of view, it is possible with just a little bit of css to keep the buttons visible despite the page height. I offer two patches for example with sticky positions on the header menu (51631.1.diff) or the footer menu (51631.2.diff).
These two patches use @supports to test the use of the css rule and also hide the unwanted button.

I understand that keeping the footer button is logical but I find that the sticky footer menu "hides" the rest of the menu and the rest of the page. It is less obvious that settings are at the bottom like the menu locations.

#7 @audrasjb
5 weeks ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for 5.7 consideration

#8 @hedgefield
2 weeks ago

  • Keywords needs-design removed

Thanks for the designs @maxpertici, those look like good options! I don't have an outspoken favorite, but I think the sticky bottom makes most sense as it also has the Delete option, which otherwise is kind of hard to discover.

@audrasjb
2 days ago

51631.2.diff – on large screens

@audrasjb
2 days ago

51631.2.diff – on medium screens

@audrasjb
2 days ago

51631.2.diff – on small screens

#9 @audrasjb
2 days ago

  • Keywords has-patch commit added; needs-patch removed
  • Status changed from reviewing to accepted

I widely tested 51631.2.diff and it looks like a great enhancement 👍

I'd lean towards committing this proposal asap in the alpha cycle, to make sure to give some time to be widely tested. On my side the proposal looks really good (see screenshots above). Plus, it was validated by the design team.

Marking this for commit.

#10 @poena
35 hours ago

I have tested 51631.2.diff and it works well for me.

@sabernhardt
13 hours ago

z-index issue with expanded menu item

@sabernhardt
13 hours ago

footer covers checkboxes (and their label) even when in focus

#11 in reply to: ↑ 6 @sabernhardt
12 hours ago

  • Keywords commit removed

Replying to maxpertici:

I understand that keeping the footer button is logical but I find that the sticky footer menu "hides" the rest of the menu and the rest of the page. It is less obvious that settings are at the bottom like the menu locations.

I might like having a sticky footer, but I've also used this screen often and know my options well. It's probably not so friendly to new users.

Is making the footer sticky enough of a benefit? If it is sticky, the z-index needs to be more than 10 to stay above the expanded menu items.

When navigating with the keyboard, the footer can cover the checkboxes, even when they are in focus. While the location options can be hidden in at least Chrome and Firefox, Chrome tends to scroll down to show them more often than Firefox.

Also, we avoided introducing a breakpoint of 783 pixels in wp-admin last year (ticket:49576#comment:26 and ticket:48780#comment:19). On the other hand, the block editor uses that breakpoint now.

I definitely prefer hiding/removing the top button and keeping the bottom one, especially in the case of a large menu combined with slow hosting and/or connection speed. (Saving a menu before all items load removes the unloaded items without a way to revert changes.)

@maxpertici
10 hours ago

@maxpertici
10 hours ago

#12 @maxpertici
10 hours ago

I propose a new patch (51631.3.diff) to take into account the remarks of @sabernhardt : the z-index and keyboard navigation.

I also added a shadow to make the footer stand out when it is above the other elements.

After thinking about new users, I tell myself that if the menu is new or short, all options will be visible.

I have no opinion to share.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


8 hours ago

#14 @ryokuhi
8 hours ago

  • Keywords needs-testing added

#15 @ryokuhi
8 hours ago

This ticket was discussed today during the weekly accessibility bug scrub.
The new patch needs a few more testing and possibily some testing using assistive technologies, but if everything is fine it can be committed.

Last edited 8 hours ago by ryokuhi (previous) (diff)
Note: See TracTickets for help on using tickets.