fix(menu-button): hide the menu button when the split pane is visible or auto hide is true

[mlynch]

This PR hides any ion-menu-button when a split pane is visible.

Currently, split pane menu buttons are invisible but take up space in the toolbar, causing the title to be misaligned:

Before:

When forcing ion-menu-button to be display: none when the split pane is visible, the alignment is fixed. The menu button properly shows again when the screen size is reduced such that the split pane collapses.

[brandyscarney]

I noticed this the other day. Looks good!

[simonhaenisch]

If I'm not mistaken, the underlying issue is that the width and height of the button have been moved to the host in #18434 (for easier customization). So I think a better fix would be to instead move the width and height values back to the button and then expose them as variables, because the split-pane might not be the only thing influencing the visibility of the button?

button {
  width: var(--width, --height, 48px);
  height: var(--height, --width, 48px);
}

What do you think? Only issue I can think of is potentially accidental inheritance of the variables because "width" and "height" are quite unspecific names?

May I also suggest adding a screenshot test for this?

[brandyscarney]

@simonhaenisch At the moment we don't have screenshot tests for desktop views. We only have them for mobile screens. Updating screenshot to take images of more screen sizes is on our TODO list.

[simonhaenisch]

@brandyscarney I meant a test for the button + header title (:

4.4.0	4.5.0
https://jsfiddle.net/simonhaenisch/31xpv57s/	https://jsfiddle.net/simonhaenisch/qon16xfd/

But great news that you'll add other viewport sizes for your tests!

[brandyscarney]

Oh! Yes I can do that @simonhaenisch. 🙂

[brandyscarney]

We need to update the code so that the menu button only has width when auto-hide is false or split pane is visible

[simonhaenisch]

@brandyscarney just wondering, why is it important again that the host element has its width set to a fixed value? Because without that constraint it would be a lot less complex to fix this in CSS, rather than forceUpdate.

[brandyscarney]

@simonhaenisch It's updated that way to match the Material Design spec: https://material.io/design/components/app-bars-top.html#specs

It's not as noticeable when viewing the spec there, but if you go to their demo you'll see that all of their buttons in a toolbar need width and height with their border-radius for the hover / focused states: https://material-components.github.io/material-components-web-catalog/#/component/top-app-bar/standard

I'm going to be looking into a different approach to do this though. I don't like this way either but I was trying it out.

[brandyscarney]

Here’s a different approach, this actually will improve accessibility: #18702

[simonhaenisch]

@brandyscarney ah ok I see, thanks!

I actually wanted to suggest the exact same idea with setting display: none using the ionMenuChange and ionSplitPaneVisible events 👌But while typing I had to get on a call and in the meantime you did it already 🤓

Looks good to me!

[brandyscarney]

Hey Max, I am going to close this as we are going to go with this approach: #18702. Thank you!