vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
462 stars 83 forks source link

[menu-bar] Remove container part from public component API #2761

Open web-padawan opened 3 years ago

web-padawan commented 3 years ago

Motivation

Follow-up from #2742

We use [part='container'] in vaadin-menu-bar mostly because of the following hack:

https://github.com/vaadin/web-components/blob/046b252d82da31ae737c55aca3d0faca1bf89c49/packages/vaadin-menu-bar/theme/material/vaadin-menu-bar-styles.js#L6-L9

It’s a non-semantic part, not something developers should theme, and we should remove that part from the API.

Proposed solution

Figure out another workaround for the Material theme and then remove container part from vaadin-menu-bar.

rolfsmeds commented 3 years ago

I wonder if we should though: as this is the parent element for menu bar buttons, it's a likely place where developers might want to apply custom css, e.g. in order to add whitespace between items, change their horizontal alignment, etc.

(Related to this is the idea of removing or reducing the need for themable-mixin-based styling of components in favor of native ::part based styling.)

Comments from @jouni and @anezthes appreciated :)

jouni commented 3 years ago

What is the role of the container? Without checking the source code, I guess it’s related to check when the items overflow? From a theming POV, I would think I can just use the host to control whitespace between items and horizontal alignment. The nested container div seems like an implementation detail I should not care about.

rolfsmeds commented 3 years ago

It's the flex container, so clearly not just for checking overflow, and very much the place you'd control the layout of the buttons:

        [part='container'] {
          position: relative;
          display: flex;
          width: 100%;
          flex-wrap: nowrap;
          overflow: hidden;
        }
jouni commented 3 years ago

Why can't the host be the flex container?

rolfsmeds commented 3 years ago

That I'm not sure about. I guess if we refactored the dom so that the host was the flex container and the current part="container" was display:contents or removed altogether, then there would be no reason to keep the part name (esp. if we remove it 😁 )