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

Bugfix: set active navigation node when smartmenus are enabled, resoves #620, #384 and #694. #706

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Menrath
Copy link
Contributor

@Menrath Menrath commented Aug 26, 2024

Another summer time theme_boost_union fix.

This targets at solving the issues #620, #384 and #694.

In the file classes/output/navigation/primary.php when smartmenus were enabled the parent:export_for_template does not get called ( parent class is core\navigation\output\primary).

Cause of the bug: The menu with the smartmenu nodes are just merged with the primary navigation and the custommenus via the PHP function array_merge.

Fix: Now the smartmenus and the custommenus are merged via array_merge and later merged with the primary menu nodes via the function merge_primary_and_custom. This function checks for the active node, the same way it happens when smartmenus are disabled.

Maybe this should also be adjusted when the variable $mobileprimarynav gets set, too.

To-Do:

  • Write new acceptance tests that cover the flag

At reviewers:
There is still a bug when clicking on external static link in the smartmenu and using the browsers return function, that multiple nodes (the actual active one and the external one) are shown as active. But I don't have any idea how to fix this edge case.

@Menrath
Copy link
Contributor Author

Menrath commented Aug 28, 2024

Something I am not completely happy with concerning the tests which is way I select the smartmenu top level menu items: .primary-navigation [data-orgposition='5'].

There seems to be no way to set the data-key for the smartmenus. Maybe one should be auto-generated by the title/text. Or is there a smarter more robust solution concerning the tests?

@Menrath Menrath marked this pull request as ready for review August 28, 2024 20:06
@Menrath Menrath changed the title Fix active navigation node when smartmenus are enabled Bugfix: display active navigation node when smartmenus are enabled, resoves #620 and #384. Aug 29, 2024
@wiebkemueller-hsh
Copy link
Collaborator

Hello @Menrath as discussed earlier, could you have a look at this issue while at it? There might be synergies. #694

@Menrath Menrath changed the title Bugfix: display active navigation node when smartmenus are enabled, resoves #620 and #384. Bugfix: set active navigation node when smartmenus are enabled, resoves #620, #384 and #694. Oct 3, 2024
@Menrath
Copy link
Contributor Author

Menrath commented Oct 4, 2024

The behat tests now fail for the newest MOODLE_404_STABLE, but I think the issue is not related with this PR.

@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants