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

Side nav issues with other routers #6468

Closed
tomivirkki opened this issue Sep 11, 2023 · 2 comments · Fixed by #7131
Closed

Side nav issues with other routers #6468

tomivirkki opened this issue Sep 11, 2023 · 2 comments · Fixed by #7131
Assignees

Comments

@tomivirkki
Copy link
Member

tomivirkki commented Sep 11, 2023

Description

If you use the <vaadin-side-nav> component with next.js, you'll notice that it doesn't work out of the box with next.js router.

The first issue is that <vaadin-side-nav-item> includes an anchor element inside its shadow root, and it can't be replaced with next/router's <Link>. Because of this, you get a full page reload whenever a side nav item is clicked.

As a workaround, you can cancel the click-events coming from the side-nav items and manually change the route:

<SideNavItem
  path="/home"
  onClick={(e) => {
    e.preventDefault();
    router.push("/home");
  }}
>
  Home
</SideNavItem>

This alone isn't enough, since the current (highlighted) side nav item will not get automatically updated on page change.

As a (hacky) workaround, since the side-nav items listen to "popstate" event, you can manually dispatch one on route change to have current side-nav item updated:

const router = useRouter();
router?.events?.on("routeChangeComplete", () => {
  window.dispatchEvent(new Event("popstate"));
});

Expected outcome

Being a general-use Web Component, the <vaadin-side-nav> should work with routers other than Vaadin's without hacky workarounds.

Minimal reproducible example

my-app.zip

Steps to reproduce

Start up the app with

  • npm i
  • npm run dev
  • Notice that the side nav works as expected.
  • Remove the workarounds (in Layout.tsx) to see the issues mentioned in the description

Environment

Vaadin version(s): 24.2

Browsers

Issue is not browser related

@yuriy-fix yuriy-fix added bug Something isn't working Impact: High Severity: Minor enhancement New feature or request and removed bug Something isn't working Severity: Minor labels Sep 13, 2023
@peholmst
Copy link
Member

This applies to the react router as well.

@tomivirkki tomivirkki self-assigned this Feb 14, 2024
@tomivirkki
Copy link
Member Author

tomivirkki commented Feb 15, 2024

We first prototyped adding a new slot for a custom anchor that would allow a user to place, for example, a react-router's Link inside a menu-bar item so that it would replace the default anchor in the component's shadow root. This turned out to be problematic mainly because of the prefix and suffix elements. A user would need to place their prefix/suffix inside the slotted anchor which in turn would make some of the built-in styles no longer apply to them. User would also need to duplicate the path and probably other side-nav item's properties for the slotted anchor as this might not be doable automatically (could lead to vdom/dom mismatch for example).

An alternative way to tackle the original problem would be to add a function property for wiring a custom router on side-nav level instead. If the callback is set, by default, the side-nav would cancel any click events originating from the item anchors in favor of invoking the callback function, which in turn would handle the navigation using router API.

The other problem with automatically updating the side-nav items on browser URL change is tricky because history.pushState, used by many client-side routers, doesn't dispatch any events. navigation API would solve the issue, but it's currently only available on Chrome.

One solution could be to add a new side-nav property, whose change observer dispatches an event that's listened by the side-nav items (in addition to "popstate").

Usage with the proposed solution (react-router):

import { SideNav, SideNavItem } from "@vaadin/react-components";
import { useNavigate, useLocation } from "react-router";

function SideNavigation() {
  const navigate = useNavigate();
  const location = useLocation();

  return (
    <SideNav onNavigate={({ path }) => navigate(path)} location={location}>
      <SideNavItem path="contacts">Contacts</SideNavItem>
      <SideNavItem path="hello">Hello</SideNavItem>
    </SideNav>
  );
}

Usage with the proposed solution (Next.js router):

import { SideNav, SideNavItem } from "@vaadin/react-components";
import { useRouter } from "next/router";

function SideNavigation() {
  const router = useRouter();

  return (
    <SideNav onNavigate={({ path }) => router.push(path)} location={router.pathname}>
      <SideNavItem path="contacts">Contacts</SideNavItem>
      <SideNavItem path="hello">Hello</SideNavItem>
    </SideNav>
  );
}

Prototype at https://github.com/vaadin/web-components/tree/feat/side-nav-item-click-callback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants