Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
While we want this component to be generic and allow all styling to be overridden, it would be nice if the default state of the menu component had a small amount of styling.
Proposed resolution
Something in the neighborhood of the default state for the example React menu component seems reasonable: https://decoupled-menu-component.jsonapi.dev/
Remaining tasks
* Style component
* Confirm that any new styling can still be overridden.
Issue fork gdwc-3206401
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
jenniferhoude CreditAttribution: jenniferhoude commentedI will start working on this during the Decoupled Menus initiative April 13th
Comment #3
jenniferhoude CreditAttribution: jenniferhoude commentedComment #5
jenniferhoude CreditAttribution: jenniferhoude commentedComment #6
brianperry@jenniferaube added a comment on the MR up above. A couple other notes:
* Could we add a little padding around the menu level? You can see that it is tight in the storybook example here: /?path=/story/components-menu--styled-by-shadow-parts
* We might have to coordinate merging this with https://www.drupal.org/project/gdwc/issues/3206410 - I'm hesitant to hide the menu children until there is a way to hover or click to show them.
* Also, we might want to consider mobile styling as well here - most likely stacking the menu items on mobile. We could address that here, or if preferred we could split that out into another issue.
Comment #7
lhockleyI am interested in helping with some of the CSS here.
Just want to make a note as we discussed during our contribution meeting today, however, that we may want to consider an alternative way to build the CSS, as these changes would dictate a horizonatal nav menu.
Suggestions include:
- Refactoring CSS to conditionally add style based on props/flags. For example, props of type=vertical, type=raw (plain list item with no CSS styles applied), type=horizontal (applies the display:flex, and padding for list items).
- Creating CSS classes that can be applied when instantiating a component
Open to other suggestions, of course.
Comment #8
jenniferhoude CreditAttribution: jenniferhoude commented@brianperry I updated the MR from your feedback.
- I added more padding around menu items.
- Kept the title of menu and is now left aligned to the menu now aligned on the right.
- Removed css to hide sub-menu, will keep have it fixed in https://www.drupal.org/project/gdwc/issues/3206410
- Mobile styling and functionality
Comment #9
jenniferhoude CreditAttribution: jenniferhoude commentedComment #10
brianperryComment #11
brianperryHoping to review sometime this week.
In the meantime, thinking longer term about theming we could consider following @castastrophe's proposed conventions for web component theming.
Comment #12
brianperryDid a little refactoring to incorporate recent work along with introducing a theme property similar to what @lhockley mentioned.
* Merged latest 1.0.x and addressed merge conflict.
* Added a new `theme` property to `gdwc-menu`. With no theme the only styling customization is that child menu items are collapsed by default. @jenniferaube's ongoing work is now triggered by `theme=horizontal`. Also added support for `theme=unstyled` which results in only raw unstyled markup.
* Added stories for horizontal and unstyled themes.
* Added detail on theme property approach to styling api documentation.
@jenniferaube still owe you feedback on your work, and hope to provide that soon. Just wanted to incorporate this refactoring first.
Comment #13
brianperry@jenniferaube made a few minor adjustments that I'm hoping will help as we work on finishing this up
* Added a default background color for the horizontal theme. We can decide if this is something we want to keep for the final version, but it makes it a little easier to see the bounds of the component. This uses a `--background` css custom property so it can be overridden by the consuming application.
* I also removed the story level padding added by storybook.
* I changed the breakpoint to desktop to 1024px. Seems like this is also something we'd wan to make possible to override. I'll create a new issue for that since it will probably require some thought and discussion.
General review notes:
* It would be nice to refine the link and hover styling here.
* We should indicate items that are expandable. https://decoupled-menu-component.jsonapi.dev/ and https://codesandbox.io/s/gdwc-vaadin-menu-bar-eqkg6 show possible approaches.
* Things get a little funky when using the horizontal theme with menus that are deeply nested. You can see this if you change the Drupal wiki example to use the horizontal theme in storybook. Would be interested if there are any ideas to improve this. Or we could conclude that this theme isn't intended for deeply nested menus.
* The hamburger menu could use some visual tweaks. It seems to be a little off center, as is the X when the menu is open. It also seems a little too large or to heavy.
* The mobile version of the menu could probably use just a little more styling when expanded. Again we might be able to use this as a model: https://decoupled-menu-component.jsonapi.dev/
* Noticed a bug - at the mobile breakpoint expand and then collapse the menu. Then resize to the desktop breakpoint. Nav items are no longer visible.
Thanks for the work thus far on this. This one requires quite a bit of trial and error to find the appropriate amount of styling balanced with the fact that we want to ensure the styling can be customized.
Comment #14
brianperryPushed up a couple of updates.
* Added simple hover styles
* Added indicator for expand and collapse state of parent menu items
Regarding:
> Things get a little funky when using the horizontal theme with menus that are deeply nested. You can see this if you change the Drupal wiki example to use the horizontal theme in storybook. Would be interested if there are any ideas to improve this. Or we could conclude that this theme isn't intended for deeply nested menus.
I think it is fine to assume that the horizontal theme isn't intended for deeply nested menus at the moment.
Comment #16
brianperryAddressed the remaining comments and merged the results. Thanks for all the help with this @jenniferaube!