Shopify / shopify-app-bridge

https://shopify.dev/docs/api/app-bridge
85 stars 9 forks source link

AppBridge NavigationMenu.Set({Active: SettingsLink}); Not Working #187

Closed elygiux closed 1 year ago

elygiux commented 1 year ago

The Navigation menu, the app bridge navigationMenu (https://shopify.dev/docs/apps/tools/app-bridge/actions/menu/navigation) is not working with navigationMenu.set({active: settings});

I use the latest versions of app-bridge (3.7.2) and app-bridge-utils (3.5.1).

Here is the code I use: https://pastebin.com/UBamXRuX

I tried both CDN version and npm module.

The highlighting works when I click on a link, but not with navigationMenu.set();

joeyfreund commented 1 year ago

@elygiux , thank you for reporting this bug.

The team has just released a fix, can you please check to see if it's working now?

elygiux commented 1 year ago

@joeyfreund It works except for navigationMenu.set({active: undefined}); It does not unselect the side menu element.

The expected behaviour would be that It would be unselected.

joeyfreund commented 1 year ago

Thanks, I'll report it back to the team and follow-up here

henrytao-me commented 1 year ago

@elygiux

Is there a specific reason you need to use active property? There are a few unexpected behavior with this property and we want to deprecate it in the future.

Unexpected behavior:

Our suggestion:

We suggest to not use active property. By default, the sidenav will highlight based on url.

elygiux commented 1 year ago

@henrytao-me We do not use react, but a mix of Laravel blade and Vue.js. Some routes are not through Javascript and do not go through the Shopify side menu. So in order to overwrite the url, we need to create separate files for Shopify app bridge redirects which is undesired thing to do and creates complexity.

Active property is important to us when using those links, but undefined or null properties are not important to us. Just wanted to report that It's not working as expected, but If It causes unexpected behaviour, the recent fix is more than enough for us!

joeyfreund commented 1 year ago

@henrytao-me, thanks for following up. @elygiux, thanks for the additional clarifications (it's super helpful for us to understand real-world use cases 🙏 ).

If you guys don't mind, I'm going to close this issue in order to keep our backlog tidy 🧹

henrytao-me commented 1 year ago

@elygiux My suggestion for your use case is as follow:

elygiux commented 1 year ago

@henrytao-me Using App Bridge History.push solved It!

Thank you!