search

Shopify / shopify-app-bridge

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

App Bridge CDN Navigation UX Lag #242

Closed panzacoder closed 5 months ago

panzacoder commented 9 months ago

Describe the bug

In an app bridge app using the latest tool of our choice NextJS 13.5 (App Router) and Shopify App Bridge (CDN), there is a UX bug when navigating between routes using the App Bridge Navigation links.

--

By digging into the app-bridge-remix starter repo, I found that you were using Remix Router Link components instead of plain <a/> tags as I had assumed was required. This provided a huge benefit! Where the plain html required a full page reload on every navigation (and loading a bounce page to obtain the bearer token), whereas navigation inside of the iframe was able to do router-based navigation, which is obviously much faster.

However, this bug is a side-affect. I will provide a screen recording, as that is the only good way to show what I mean. It should be noted that navigating within the iframe still populates the side-bar nav active link styling, but navigating from the actual sidebar has the styling lagging behind. My guess is that this is some sort of listener that exits too early, but I can't see your code for app bridge so ¯_(ツ)_/¯ .

To Reproduce

I currently cannot provide a minimal repro due to the code being private, but I will try to provide one shortly if it is helpful.

If applicable, add screenshots to help explain your problem.

https://github.com/Shopify/shopify-app-bridge/assets/29257545/5d19a1f9-adf2-48d6-9027-6409811432dc

Expected behaviour

Navigation should be both quick and keep active nav styling in sync, as it does with iframe routing otherwise.

Contextual information

Packages and versions

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

Platform

Additional context

bithaolee commented 9 months ago

I'm experiencing the same problem, when I open my app directly through the app link or refresh the whole app, it triggers this effect and I need to double-click to select the menu, but if I'm clicking through the app directory to enter the app, it doesn't trigger the bug, this problem exists for a month or two now, and I don't know how to resolve it

paweltatarczuk commented 9 months ago

I experienced the same issue with Vue Router. A workaround is to ensure that the pushState happens synchronously.

A working example in Vue:

<template>
  <ui-nav-menu>
    <a href="/" rel="home" @click.prevent="navigate($event, '/')">Home</a>
    <a href="/page" @click.prevent="navigate($event, '/page')">Page</a>
  </ui-nav-menu>
</template>

<script setup lang="ts">
import { useRouter } from "vue-router";

const router = useRouter();

function navigate(event: MouseEvent, to: string) {
  history.pushState({}, "", to);
  router.replace(to);
}
</script>
panzacoder commented 9 months ago

I experienced the same issue with Vue Router. A workaround is to ensure that the pushState happens synchronously.

Trying to reason about what an equivalent solution would be for me. I'm not familiar with the @click.prevent syntax, are you consuming the click event and then calling your own javascript instead?

If so I think an equivalent would be:

<Link 
  href="/" 
  rel="home" 
  onClick={
    (e) => {
      e.preventDefault(); 
      router.push("/");
    }
  }
/>

I know you may not be that familiar with React, so just thinking out loud here. I'll try it out soon.

paweltatarczuk commented 9 months ago

Yes, that's a correct equivalent as long as the router.push("/") is triggering history.pushState() in the same tick. As you can see, Vue Router is not modifying the history in the same tick, which is why I'm pushing the state manually here and using router.replace() instead of router.push(). I'm not sure what the correct approach is in React. What I do know is that the remix starter template is working as expected with <Link> without any workarounds.

tommypepsi commented 7 months ago

I managed to make it work using @panzacoder idea. Though the prevent default was simply preventing the navigation altogether. Removing the prevent default was working but causing a full page refresh with just the router.push("..."). So I changed that for history.pushState(null, "", "/") and it worked perfectly! There's still a little flash of the navigation but now it at least highlight the right menu in the end.

Here's what worked in the end for me:

<ui-nav-menu>
  <Link 
    to="/app" 
    rel="home"
    onClick={() => {
        history.pushState(null, "", "/app");
    }}
  >
    {t("menu.home")}
  </Link>

  <Link 
    to="/app/settings"
    onClick={() => {
        history.pushState(null, "", "/app/settings");
    }}
  >
    {t("menu.settings")}
  </Link>

  <Link 
    to="/app/setup"
    onClick={() => {
        history.pushState(null, "", "/app/setup");
    }}
  >
    {t("menu.how_to_setup")}
  </Link>

  <Link 
    to="/app/general-info"
    onClick={() => {
        history.pushState(null, "", "/app/general-info");
    }}
  >
    {t("menu.general_information")}
  </Link>
</ui-nav-menu>
panzacoder commented 7 months ago

@tommypepsi Thanks for the bump! I was actually able to achieve this with pretty much the same code, although I also deduplicated a bit. Feel free to take it!

'use client';

import NextLink, { LinkProps } from 'next/link';

function Link({ href, onClick, ...props }: LinkProps & { children: React.ReactNode; rel?: string }) {
  return (
    <NextLink
      href={href}
      onClick={(e) => {
        history.pushState(null, '', href.toString());
        onClick?.(e);
      }}
      {...props}
    />
  );
}
export default function Navigation() {
  return (
    <ui-nav-menu>
      <Link href="/" rel="home">
        Home
      </Link>
      <Link href="/page-1">Page One</Link>
      <Link href="/page-2">Page Two</Link>
      <Link href="/page-3">Page Three</Link>
    </ui-nav-menu>
  );
}

I'll leave this thread open for visibility, but if the App Bridge team thinks this is an acceptable solution, that's cool. I think at this point the ask would just be some additional documentation that if your router and app bridge isn't in sync, you'll need to manually call history.pushState().

henrytao-me commented 5 months ago

We deployed a fix for this. Let us know if you still see anything weird. 🙇

panzacoder commented 5 months ago

@bartcheers

tobiasdalhof commented 3 months ago

I experienced the same issue with Vue Router. A workaround is to ensure that the pushState happens synchronously.

A working example in Vue:

<template>
  <ui-nav-menu>
    <a href="/" rel="home" @click.prevent="navigate($event, '/')">Home</a>
    <a href="/page" @click.prevent="navigate($event, '/page')">Page</a>
  </ui-nav-menu>
</template>

<script setup lang="ts">
import { useRouter } from "vue-router";

const router = useRouter();

function navigate(event: MouseEvent, to: string) {
  history.pushState({}, "", to);
  router.replace(to);
}
</script>

@paweltatarczuk Thank you, that works. But the navigation menu is not always updated when you change the route via other components. To fix this, you can re-render the menu by adding a key property that is updated when the route is changed

<template>
  <ui-nav-menu :key="key">
    <a href="/" rel="home" @click.prevent="navigate($event, '/')">Home</a>
    <a href="/page" @click.prevent="navigate($event, '/page')">Page</a>
  </ui-nav-menu>
</template>

<script setup lang="ts">
import { useRouter } from "vue-router";
import { ref, watch } from 'vue';

const router = useRouter();

function navigate(event: MouseEvent, to: string) {
  history.pushState({}, "", to);
  router.replace(to);
}

const key = ref(0);
watch(router.currentRoute, () => key.value++);
</script>