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

Dynamically assign active-menu-item to the menu #68

Merged
merged 5 commits into from Jan 13, 2022
Merged

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Jan 11, 2022

This is probably not the most efficient way, but is more dynamic than what we currently use on WordPress.org (which is mostly a tightly coupled if-branching setup).

Some style changes were required to make submenu active highlighting work more correctly, see below screenshots of the after effect.

Screen.Recording.2022-01-11.at.3.22.22.pm.mov

And for the reasoning for the CSS rotation:

Screen Shot 2022-01-11 at 3 20 20 pm

See #35

@dd32 dd32 requested a review from iandunn January 11, 2022 05:24
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

This looks good 👍🏻

The only thing I might change is that get_menu_url_for_current_page() feels a bit complicated, but I don't have any ideas for simplifying it. So I'd go ahead and merge 👍🏻

@dd32
Copy link
Member Author

dd32 commented Jan 12, 2022

The only thing I might change is that get_menu_url_for_current_page() feels a bit complicated

@iandunn I also felt that, which is why I extracted the logic from set_current_item_class() directly to start with (and it was simpler then..).

I'd love to simplify this in future, I just wasn't able to figure out anything more reliable given the menu doesn't know if it's pointing to get_permalink(), home_url('/'), or a combination of them. Might be able to add some flags into the menu array to start with, but then Rosetta dynamic menu's kicks in and throws a spanner in there :)

@iandunn
Copy link
Member

iandunn commented Jan 12, 2022

👍🏻

@dd32 dd32 merged commit 6e9673a into trunk Jan 13, 2022
@dd32 dd32 deleted the add/dynamic-active-menu branch January 13, 2022 01:26
dd32 added a commit that referenced this pull request Jan 24, 2022
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.

None yet

2 participants