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

Fixes to horizontal menu logic. Now supports two levels of menu items. #1074

Merged
merged 4 commits into from
Jan 28, 2021
Merged

Fixes to horizontal menu logic. Now supports two levels of menu items. #1074

merged 4 commits into from
Jan 28, 2021

Conversation

Jayson-Furr
Copy link
Contributor

@sbwalker I have modified the horizontal menu logic to include two levels of menu items. It now renders the item correctly. I'll start on the vertical now and PR it next.

@hishamco
Copy link
Contributor

@Jayson-Furr could please show us screenshot for how the menus are look like

@Jayson-Furr
Copy link
Contributor Author

ss1
ss2
ss3

@Jayson-Furr
Copy link
Contributor Author

image
image

@Jayson-Furr
Copy link
Contributor Author

I think there is room for improvement on the horizontal menu style. I think a top level page that is navigation with children should be an empty page with the redirect setup for the first child in it's page management for the flow to work well. Otherwise the top level is a link and menu activator which can cause the page / link to behave strange. Some clicks cause the menu to expand while others cause a navigation. If that makes sense. If we remove the link from the parent and just use the top level page as a page container then that would probably suffice.

@Jayson-Furr
Copy link
Contributor Author

What do you think about adding a menu system separate from the pages system? Maybe multiple menus and a page could be added to a menu? On the page management, have toggles for each menu vs IsNavigation? Then the menu's could be added to the PageState similar to the Pages property? This would allow a setup of multiple menus for a site, example Navigation, Footer, Administration, etc.

@Jayson-Furr
Copy link
Contributor Author

Yet another option, each page could have an optional menu control type that renders it and/or it’s children. This would allow the theme to have menu types, examples no sub menu, hover menu, drop down menu, mega style menu, etc.

@hishamco
Copy link
Contributor

Yet another option, each page could have an optional menu control type that renders it and/or it’s children

I think the page already have a property called HasChildren or something similar

@sbwalker
Copy link
Member

This is a very good improvement to the menus and it also fixes the bugs identified previously. A few additional comments:

  • since Oqtane is a modular framework and not a CMS, it’s focus is to provide the basic building blocks and extensibility points so that developers can create custom applications. The current Page model and menu components satisfy this goal. If someone wants to create an application with a highly sophisticated navigation system with a specific user experience or use multiple navigation menus, etc... they can create a custom theme and create custom menu components - it is not the core framework’s responsibility.

  • one of the items in the Oqtane philosophy is consistency... and one of the patterns we are following throughout the core framework UI is the inline razor approach rather than the code behind approach. I will accept this PR in its current form but will convert the components to inline so they are consistent ( unless you want to convert them prior to merging ). Developers are obviously free to use the code behind approach in their own custom modules and themes.

@sbwalker sbwalker merged commit 9ba2328 into oqtane:dev Jan 28, 2021
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