matcher function of <NavigationMenu/> not behaving as expected

hey-servicify commented 1 year ago

Describe the bug

When using the matcher function on a component, certain navigation menu items are marked as highlighted even though the matcher function returns false.

To Reproduce

Steps to reproduce the behaviour:

You can see the behavior on my dev store here: https://admin.shopify.com/store/servicifyzoomtest1/
Use the following credentials to login: Email: tilomitra+zoom@gmail.com, Password: zoomdeveloper
Once logged into the store, go to "Apps" > "Easy Appointment Booking"
From the "Dashboard" menu item, click on the "Manage Event" button.
Notice how the NavigationMenu component automatically highlights "Custom Fields". This is done despite the matcher function being explicitly set to false.

If applicable, add screenshots to help explain your problem.

Expected Behavior

The custom fields menu item should not be selected. No menu item should be highlighted.

Contextual information

Here is what the NavigationMenu is rendered as:

<NavigationMenu
        navigationLinks={[
          {
            label: "Dashboard",
            destination: "/?tab=dashboard",
          },
          {
            label: "Bookings",
            destination: "/?tab=bookings",
          },
          {
            label: "Availability",
            destination: "/?tab=employees",
          },

          {
            label: "Account ⭐",
            destination: "/?tab=pricing",
          },
          {
            label: "Settings",
            destination: "/?tab=settings",
          },

          {
            label: "Custom Fields",
            destination: "/?tab=custom-fields",
          },

          {
            label: "Notifications",
            destination: "/?tab=notifications",
          },

          {
            label: "Widgets",
            destination: "/?tab=widgets",
          },

          {
            label: "Support",
            destination: "/?tab=support",
          },
        ]}
        matcher={(link, location) => {
          const tab = link.destination.split("tab=")[1];
          if (tab === "dashboard") return location.pathname === "/";
          else return tab === location.pathname.split("/")[1];
        }}
      />

On the page in the screenshot, the URL is /event/:id/detail, so the matcher() function does return false for every link. Despite this, the Custom Field link is somehow selected. We noticed that it always selects the last visible link on the page.

Packages and versions

List the relevant packages you’re using, and their versions. For example:

    "@shopify/app-bridge": "^3.7.5",
    "@shopify/app-bridge-react": "^3.7.5",
    "@shopify/app-bridge-utils": "^3.5.0",
    "@shopify/polaris": "^10.43.0",
    "@shopify/polaris-icons": "^4.8.0"

Platform

OS: MacOS (although I see it on Windows too)
OS Version: 13.0 (Ventura)
App: Chome v113

Additional context

App's configuration is:

NextJS Server-renders a single page app with React routing
React Router client rendering
Koa backend on NodeJS v16

henrytao-me commented 1 year ago

@hey-servicify Your matcher is wrong. It should be as below based on your setup.

The matcher function will loop through all the items giving the current locationOrHref after navigation happens. So, you need to extract the target from locationOrHref instead of the link.

                matcher={(link, locationOrHref) => {
                  const location =
                    typeof locationOrHref === "string"
                      ? new URL(locationOrHref)
                      : locationOrHref;
                  return (
                    link.destination === location.pathname + location.search
                  );
                }}

There is a minor issue with the navigation item highlight flashing which is a separate issue.

hey-servicify commented 1 year ago

Hi @henrytao-me thanks for getting back to me. I don't believe the solution proposed is actually solving the issue. The simplest way I can put it is this:

Even if I make my matcher function the following:

matcher={(link, locationOrHref) => {
  return false;
}}

A navigation item is still being selected. It's usually the last one in the list. I would assume this is a bug, right? Similarly, I have noticed that the matcher() is actually not running through all the items in the loop. If I console.log(location) in that function, I only get 2 results back, but I have 9 nav items.

henrytao-me commented 1 year ago

Hi @hey-servicify

I think there are several things happening here:

[Bug on our side]: matcher doesn't work and last item (before expanding) is highlighted when NavigationMenu uses query params (Ex: ?tab=nextPage) instead of pathname (Ex: /nextPage).
[None bug]: you only get 2 calls while your app has 9 nav items. The reason for this is that App Bridge calls matcher and stops calling after the first return true.

I think we could schedule a pairing on partner slack if you prefer.

hey-servicify commented 1 year ago

Hey @henrytao-me a pairing session sounds great. What would be the best way to schedule this with you?

henrytao-me commented 1 year ago

Maybe sometimes after 1pm EST next Monday June 19th?

hey-servicify commented 1 year ago

Hey @henrytao-me sorry for missing the notification above. What's your availability this Friday June 23?

henrytao-me commented 1 year ago

Sure.

hey-servicify commented 1 year ago

@henrytao-me Sorry, im not sure of the best way to get in touch with you outside of Github haha. Can I get an invite to the Shopify partner slack? Alternatively, what email address should I use for an invite?

hey-servicify commented 1 year ago

hey @henrytao-me just following up on my request above

henrytao-me commented 1 year ago

Oh hi @hey-servicify , you can access partners slack from here

zhou0219 commented 1 year ago

啊，你好@henrytao-me，When I use NavigationMenu,

        <NavigationMenu
          navigationLinks={[
            {
              label: t("NavigationMenu.pageName"),
              destination: "/pagename",
            },
          ]}
        />

it appears

AppBridgeError2 {name: 'AppBridgeError', message: 'APP::ERROR::PERMISSION', action: {…}, type: 'APP::ERROR::PERMISSION', stack: 'AppBridgeError: APP::ERROR::PERMISSION'} action : {type: 'APP::ERROR::PERMISSION', group: 'Error', payload: {…}} message : "APP::ERROR::PERMISSION" name : "AppBridgeError" type : "APP::ERROR::PERMISSION" stack: "AppBridgeError: APP::ERROR::PERMISSION" [[Prototype]] : Error I want to know what permissions should be added to solve this problem

henrytao-me commented 1 year ago

Is your app a channel? app-bridge-react doesn't have component for ChannelMenu. So, you might want to use app-bridge to set it up if this is the case.

Shopify / shopify-app-bridge