Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable mobile navigation #393

Merged
merged 7 commits into from
Jun 21, 2024

Conversation

lenadax
Copy link
Contributor

@lenadax lenadax commented Jun 11, 2024

Description

This pull request enhances the Mobile Navigation component to be more customizable. Key changes:

  • Refactoring the Mobile Navigation component for improved configurability.
  • Introducing a new MenuItem component, which is extracted and customized from the original MobileNavigation.
  • Introducing a new file MobileNavigationToggler containing the default Hamburger Menu as a separate component.

Motivation/Context

These changes are intended to make the Mobile Navigation component more adaptable to different use cases and easier to extend or override.

  • Flexible Nav Depth:
    The MobileNavigation can now cycle through more than three nav levels, based on navDepth configuration in the theme's index.js file. The default behavior of the theme will not change and is still limited to three levels.
    This adapted behavior reduces code length and allows the possible usage of the component as a main navigation for more nav levels, without being limited by VLT's header space requirements.

  • Customizable Burger Menu:
    Creating a separate file MobileNavigationToggler containing the burger menu leaves developers an option to easily override the default animated Burger Menu element without overriding the entire MobileNavigation.

  • Consistent positioning:
    As I was working on the MobileNavigation, I noticed the MobileNavigation Search Icon and Header were not consistently positioned when entering/leaving a submenu. Minimal css changes in this PR ensure correct positioning throughout submenues.

Please review the changes and let me know if there are any improvements or adjustments needed.

@sneridagh
Copy link
Member

sneridagh commented Jun 11, 2024

@lenadax Do this PR modify the existing HTML structure of the mobile menu or have any change in the classnames, etc? We consider these changes a breaking change, and if so, we should issue it. Basically we follow the same policy as in Volto for definition of breaking.

I will have a closer look now.

@lenadax
Copy link
Contributor Author

lenadax commented Jun 11, 2024

@lenadax Do this PR modify the existing HTML structure of the mobile menu or have any change in the classnames, etc? We consider these changes a breaking change, and if so, we should issue it. Basically we follow the same policy as in Volto for definition of breaking.

I will have a closer look now.

Yes, in order to make the Hamburger menu overridable, an additional container is required. Since the Burger Menu styles come from Volto and are applied to .hamburger elements, I had to change the class hamburger to hamburger-toggler and include an additional hamburger element inside the MobileNavigationToggler component.

Otherwise, the styles (::before and ::after) would still be applied to the button if the template is customized with another icon.

@sneridagh sneridagh requested a review from danalvrz June 11, 2024 13:21
Copy link
Contributor

@danalvrz danalvrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lenadax it looks very good, thank you for your work. There are only a couple minor things we should maybe adjust before merging, please take a look at the comments.

Even though most of the HTML was not changed, we are introducing a wrapper that could be breaking for a project that depends on the previous markup. I think this PR is a good change, just to keep that in mind. cc @sneridagh

@lenadax lenadax requested a review from danalvrz June 17, 2024 10:04
@sneridagh
Copy link
Member

@lenadax merging this now! Thanks for your contribution! I hope it's the first of many others!

FYI, we decided to ship this as a breaking, I think it's important to stick to the definition of breaking. I will take care of it when I make the release, therefore this will be in 4.x series.

@sneridagh sneridagh merged commit 5c9419a into kitconcept:main Jun 21, 2024
3 checks passed
sneridagh added a commit that referenced this pull request Jul 2, 2024
* main: (25 commits)
  Upgrade to a39, enable new image widget (#405)
  Finish interrupted push
  Fixes #400, install in Volto 17
  Fix Invalid html structure in caption component (#401)
  build deps
  Upgrade to Volto a37 (#403)
  Release 4.0.0
  Fix release-it script
  Changelog
  Fix tabbing in header (reapplies #346 as breaking) (#374)
  Customizable mobile navigation (#393)
  Fix Description block width in Edit/Add (#394)
  Slight improvements
  Adjust height
  Add BMv3 ready logo
  Remove not needed dockerfiles folder
  Update setup. Use new images. (#390)
  fix Logo alt-Title + add German translations (#337)
  fix link in introduction block being smaller than normal text (#366)
  Release 3.3.2
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants