solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.13k stars 143 forks source link

[Feature ✨] Support for contextual modal navigation #129

Open manoldonev opened 2 years ago

manoldonev commented 2 years ago

I apologize if I am posting in the wrong place but https://github.com/solidjs/solid-app-router/discussions gives me 404.

Basically I was trying to implement contextual modal navigation where the navigation path the user takes determines if the page is rendered in the modal or not -- basically like in this React example. However, it seems setting location like this is not possible with solid-app-router. Am I missing something or simply this is not supported at the moment?

Thanks!

gitcatrat commented 1 year ago

For us this is a must before even considering Solid, we heavily rely on multiple locations being rendered at the same time.

Any ideas if this router plans to support scoped Routes or are there any Solid workarounds for this?

The concept should be simple, did it in my own 1.5KB react/preact router but I'm a total Solid n00bie at this point. How I did it and I'm pretty sure react-router did as well:

// typical react/preact router using this concept
function Routes() {
  const realLocation = useLocation();
  let backgroundLocation = null;

  if (realLocation.state?.background && realLocation.state?.modal) {
    // previous location (or any really, but previous is most common) is stored to `location.state`
    // for example, links that open href in modal might look like this:
    // `<Link to="/foo" state={{ background: /* value from local `useLocation()` */, modal: true }}>..`
    backgroundLocation = realLocation.state.background;
  }

  return (
   <>
     {/* if we have a background location, use that instead of global */}
     <Routes location={backgroundLocation}>
        {/* "normal" routes go here */}
     </Routes>

     {/* if background exists, it's safe to assume that we can render modal routes */}
     {backgroundLocation && (
        <Routes>
          {/* modal routes go here, they render on top of "normal" routes */}
        </Routes>
     )}
   </>
  );
}
thetarnav commented 1 year ago

I may not understand exactly what's necessary here, but isn't it already possible to achieve similar outcomes already? https://stackblitz.com/edit/github-yjvhnz?file=src/App.tsx

There are probably better ways to accomplish it than what I've used, but I am not that familiar with the router API.

millette commented 1 year ago

@manoldonev weird, https://github.com/solidjs/solid-app-router/discussions redirects to https://github.com/solidjs/solid-router/discussions for me.

gitcatrat commented 1 year ago

@thetarnav You hit the nail in your example - it works as desired for that use case!

But it seems to be the simplest case: routes only contain one background route that can be "under modal" and one route that can be modal, I don't really understanding how it would scale well past that (based on my limited Solid knowledge).

If you look at this example: any of the modal routes should be able to render on top of any of the "normal" routes. For example you should be able to open <Login /> modal on top of any page, same goes for all other modal routes.

// based on my previous code example
<Routes location={backgroundLocation  || realLocation}>
  {/* "normal" routes */}
  <Route path="/login" />
  <Route path="/register" />
  <Route path="/user/:id" />
  <Route path="/post/:id" />
  <Route path="/posts" />
  <Route path="/users" />
</Routes>

{backgroundLocation && (
  <Routes>
    {/* modal routes */}
    <Route path="/login" />
    <Route path="/register" />
    <Route path="/user/:id" />
    <Route path="/post/:id" />
  </Routes>
)}
thetarnav commented 1 year ago

Ok, I think I understand the issue now. And yeah, my solution wouldn't scale too much. Adding this to the package would be the way to go, and it shouldn't be hard to do either, since it just replaces what the router passes via context.

Brendan-csel commented 1 year ago

Just curious if this is really a router thing? I mean, you would never be able to deep link to a page with the modal open - since the modal state isn't part of the url.

Would it not be better to either: a) have the desired model identified in the url (eg query param) and have some top-level component watch for that and render the relevant modal. b) if deep-linking is not required, skip the router and just have a "modal context" to set the desired modal - and again some top-level component watches for that and renders the relevant modal.

Your top-level modal selection component would just be a big <Switch> <Match> list.

I guess I'm just not appreciating the benefit of trying to do this with the router?

gitcatrat commented 1 year ago

@Brendan-csel I'd like to think that, all the routers I've used supported this. I first discovered it in react-router example many-many years ago and my feeds haven't looked back since.

The fact that modal information is not kept in URL is kind of the point, it provides context sensitivity.

Take this example:

The same routing concept is used in almost all popular feed-heavy sites that I've checked, e.g Reddit.

Brendan-csel commented 1 year ago

Would it be as simple as adding a location accessor to <Routes> like this https://github.com/cselnz/solid-router/commit/72aa6bd93d0bb99499a498292b80eb4d08ce81d3

@gitcatrat if you've got an app set up with the whole "background" thing - you might be able to pull that branch and build it to test. Alternatively you could probably just hack your local built js with those few changes to see if it works.

thetarnav commented 1 year ago

Why accessor? props can be getters

gitcatrat commented 1 year ago

I'll throw an example app together later today (finished Solid tutorial yesterday, haha). My thinking is still very much stuck in component re-render paradigm, so it definitely needs to be verified.

My main concern is this:

// - previous location was `{pathname: "/posts"}`
// - new `backgroundLocation` is also `{pathname: "/posts"}` taken from state,
//   which is stringified on `<a>` elements, so the reference is definitely lost

// does it flag the whole sub-route code and DOM for re-run and re-mutations or not..
<Routes location={backgroundLocation  || realLocation}>
  {/* "normal" routes */}
</Routes>
gitcatrat commented 1 year ago

Okay, tested the proposed solution out (hopefully correct based on last few comments):

// library's Router change
const matches = createMemo(() => getRouteMatches(branches(), props.location?.pathname || router.location.pathname));
// routes definition
<div>
  <Routes location={bgLoc()}>
    {/* "normal" routes */}
  </Routes>

  <Routes>
    {bgLoc() && (
      <>
        {/* modal routes */}
      </>
    )}
  </Routes>
</div>
// discovered that conditional `<Routes` is not supported or it's broken,
// produces TypeError: Cannot read properties of undefined (reading 'path')
// in function `createRouteContext`, code: `const path = createMemo(() => match().path);`
{bgLoc() && (
  <Routes>
    {/* modal routes */}
  </Routes>
)}

This does keep the background route in place but that's pretty much it. In order for all the <Routes location={custom}> descendant hooks and components to get the right location (a.k.a the one given as prop), new context needs to be set.

In other words if any of the <Routes location={custom}> active routes use useLocation() or any other hooks, they currently do not use/return location={custom} but they should be, that's kind of the point of location prop: whole subtree uses it.


Here I made a fully fledged demo https://stackblitz.com/edit/github-yjvhnz-jumkei?file=src%2FApp.jsx It showcases all the features this topic entails (if it only currently worked - uses latest solid-router).

PS! If you look at the demo, press "Open in New Tab". Stackblitz seems to struggle with navigation otherwise.

Brendan-csel commented 1 year ago

Why accessor? props can be getters

Middle of the night brain fade.

I'm just starting a busy day here today - but will have a look tonight if no-one else has.

Brendan-csel commented 1 year ago

Yeah I see there is more involved than my late-night stab from yesterday. I'll step out of this for now.

I know there are a few other things in the router that need fixing - probably before adding another level of indirection for this. The core team is just so tapped out working on solid start stuff...

gitcatrat commented 1 year ago

If this router were to support it, then probably this is what would need to change.. My Solid knowledge is still very limited but I'll try to research as much as possible for feature consideration.

<Router> {/* wraps with `RouterContextObj` - here's where `location` currently is taken from */}

  <Routes> {/* wraps with `RouteContextObj` */}
    {/* currently `useLocation()`, etc inside `<Foo />` get `location` from `RouterContextObj` */}
    <Route path="/foo" component={Foo} />
  </Routes>

  <Routes location={customLocation}> {/* wraps with `RouteContextObj`, should set `customLocation` */}
    {/* `useLocation()`, etc inside `<Foo />` should get `location` from `RouteContextObj` */}
    <Route path="/foo" component={Foo} />
  </Routes>

</Router>
mdynnl commented 1 year ago

https://playground.solidjs.com/anonymous/85febdff-51c0-426b-a1e8-ca8ec5436170

This was possible since <Router /> accepted source.

You can use pathIntegration for initial source but the source signal in createIntegration currently does a.value === b.value. Maybe it could accept custom { equals }.

xalidmalik commented 11 months ago

I have solved it for now with an internal method, I share the implementation below

import { createStore } from "solid-js/store";

function pushToUrl(url: string) {
  window.history.pushState(undefined, "", url);
}

export type ActionParallelKeys = "action-details";
export type ExploreParallelKeys = "topic-details";
export type ProfileParallelKeys = "user-details";
export type AuthParallelKeys = "login" | "register";

export type ParallelKeys =
  | ActionParallelKeys
  | ExploreParallelKeys
  | ProfileParallelKeys
  | AuthParallelKeys;

export type RouteState = {
  to: string;
  identifier: string | AuthParallelKeys;
  key: ParallelKeys;
  prevUrl: string | null;
  props?: any;
};

function useParallelRouter() {
  const [routes, setRoutes] = createStore<RouteState[]>([]);

  const _find = (identifier: string) => {
    return routes.find(route => route.identifier === identifier);
  };

  const getRoutes = (key: ParallelKeys) => {
    return routes.filter(route => route.key === key);
  };

  const getRoute = (identifier: string) => {
    return _find(identifier);
  };

  const push = async (route: RouteState) => {
    const findExistRoute = _find(route.identifier);

    if (findExistRoute) {
      return;
    }
    pushToUrl(route.to);
    setRoutes(prevRoutes => [...prevRoutes, route]);
  };

  const discard = (key: ParallelKeys, identifier: string) => {
    const findExistRoute = _find(identifier);

    if (!findExistRoute) {
      return;
    }
    pushToUrl(findExistRoute?.prevUrl!);
    setRoutes(routes =>
      routes.filter(
        route => route.key === key && route.identifier !== identifier
      )
    );
  };

  const discardAll = () => {
    setRoutes([]);
  };

  const discardLastRoute = () => {
    setRoutes(prevRoutes => prevRoutes.slice(0, -1));
  };

  const getProps = (key: ParallelKeys, identifier: string) => {
    return routes.find(
      route => route.key === key && route.identifier === identifier
    )?.props;
  };

  const setProps = (key: ParallelKeys, identifier: string, props: any) => {
    setRoutes(
      routes => routes.key === key && routes.identifier === identifier,
      "props",
      prevProps => ({ ...prevProps, ...props })
    );
  };

  return {
    push,
    discard,
    discardAll,
    getProps,
    setProps,
    getRoutes,
    getRoute,
    discardLastRoute,
  };
}

export const p = useParallelRouter();

to support the browser's back button, I use it in root

if (isHydrated()) {
    createEventListener(window, "popstate", e => {
      p.discardLastRoute();
    });
  }

For recursive use in the example:

export function ViewActionParallelModal() {
  return (
    <For each={p.getRoutes("action-details")}>
      {route => {
        const handleOpenChange = () => {
          p.discard("action-details", route.identifier);
        };
        return (
          <Modal open={!!route.identifier} onOpenChange={handleOpenChange}>
           ...Modal Content
          </Modal>
        );
      }}
    </For>
  );
}

For normal use in the example:

export function LoginModal() {
  const handleOpenChange = () => {
    p.discard("login", "login");
  };

  return (
    <Modal
      open={!!p.getRoute("login")?.identifier}
      onOpenChange={handleOpenChange}
    >
           ...Modal Content
    </Modal>
  );
}