-
Notifications
You must be signed in to change notification settings - Fork 54
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
Feature: routed modals + parameters in modal routes #121
Comments
That's interesting! Could you share that via StackBlitz for example? I'll be happy to have a look into that. I was also thinking recently to store current modal as a search params instead of route state to have it shareable. |
I can set on up later. But after more exploration, it seems like the best way to add routed modals would be to add them as children to the parent, and rendering through an Check out this example from the official repo: https://github.com/remix-run/react-router/blob/dev/examples/modal-route-with-outlet/src/App.tsx That would also support cases for routed tabs, etc. I don't think there's a convention to add nested routes right now, is there? Maybe something like index+modal.tsx? |
After more testing, seems as if nesting works with, e.g. this structure:
The only problem is what if I want the index page to also have child routes? That doesn't seem to be possible. The only option is to define I think a good option would be to treat an index directory as a source for children for the index route. Right now if you would create an index directory in e.g. companies, then it would create an erroneous route group with path "/", and that's not valid: Update:
|
I think this example https://github.com/oedotme/generouted/tree/main/examples/react-location/modals might be using a similar approach to what you're referring to. Basically to have an outlet and a child modal component, which gets mounted on top of it's parent. If I recall correctly it had some limitations and it's wouldn't be global by default. It worked tho while I was testing that, and I guess it can be used right now with nested layouts. Currently the global modals work well for me, but I'm interested to explore ways to improve it. |
I am actually trying to do this right now. Is this something you would be interested in implementing? @oedotme I am trying to do the following: Kanban board at click on a card on the board and then a draw appears with the card information at |
@archiekd modal routes can be achieved with route layouts + certain structure + just links and can be done with or without I added an example for react-router with route modals. In this example, Here's the online example on StackBlitz Hope that helps! |
I managed to get it working mostly on my end without major modifications to generouted, but there's one use case still not properly supported. There's no way to add child routes to the index page. Say you have routes for a tabbed view:
Say I want to add a modal to the gallery page/tab and render the tab still in the background. For other tabs it's easy – add a folder
|
All of your suggestions seem very delicate to me. I don't really understand if we are all trying to achieve the same goal, but I would rather explain my point of view on the current flows of the modal system. In the context of my application, modals are sometimes shared between routes: I'd like to have a modal that deletes an item based on a given ID. To achieve this, I can do the following for a route: In fact, your tricky way would work by setting a route page that is actually a modal (using the Solution : I like the idea of giving modal parameters in their filename too, like you gave in the example: |
So I found a strange way to deal with modal parameters on arbitrary routes by using history state :
function DeleteProductModal() {
let { id } = useParams("/products/:id");
if (!id) id = history.state.usr.product.id;
(...)
}
(...)
<DropdownMenuItem
className="hover:!bg-destructive hover:!text-destructive-foreground"
onClick={() => modals.open("/products/[id]/delete", { state: { product: { id: product.id } } })}
>
<TrashIcon className="mr-2 h-4 w-4" />
Delete
</DropdownMenuItem>
(...) Perhaps this is an interesting idea to include in generouted? Maybe use the URL parameters, or else look in the history state? |
Well, what you're trying to achieve is a confirm box rather than a modal. And there's no reason to put it in a route to begin with – it's not like you want or need a shareable link for a confirm dialog (imagine a situation where deletion fails for some reason, if all else fails you'd rather want the user to get rid of it with a hard refresh rather than meeting the same bug over again). I think you're much better of just having a reusable confirm dialog component that defines the the ID on the trigger. You'll find it much easier to implement and don't have to have any hacky workarounds with IDs. To that tune, I also personally don't think that global modals that don't have a URL should be handled by a routing library – they don't have a route, do they?! Also, they may need additional state and if the state doesn't live in the URL, they become finicky very easily. In that case you're also better off having reusable modal component for global modals that are tied to their triggers rather than awkwardly living in an additional routing tree / history.state. Radix has good implementations for both that help you appreciate the paradigm: I would love to avoid having to add individual outlets for routed dialogs (albeit there's nothing particularly wrong with that approach, as you can have nested Outlets for many things, such as tabs, and other content(1) – it's just the additional boilerplate that bothers me, as modals are often placed in a portal anyways), but I'm yet to come to a better alternative which enables to properly store the state in and retrieve the state from the URL, without creating alternative navigation logic on top of -- |
Loving this package so far. I generally prefer to have modals routed though rather than state based in order to have shareable links as a default, rather than state-based (which are only good for global routes).
I have managed to do this myself with custom routes file:
E.g., I have a route in
pages/companies/+edit.[id].tsx
And then in routes file I do this to render modals for routes, following the internal matching engine of react-router-dom:
Would be great to have this with type-safety out of the box for linkable routes with params.
The text was updated successfully, but these errors were encountered: