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

Don't be dependent on react-router-dom #93

Closed
KaiSpencer opened this issue May 8, 2022 · 6 comments · Fixed by #94
Closed

Don't be dependent on react-router-dom #93

KaiSpencer opened this issue May 8, 2022 · 6 comments · Fixed by #94
Assignees
Labels
🚀 enhancement New feature or request
Projects

Comments

@KaiSpencer
Copy link

KaiSpencer commented May 8, 2022

Is your feature request related to a problem? Please describe.
Im attempting to use the sidebar component, but useLocation is called internally to the component from react router. Additionally the react router Link component is used in each sidebar item.

Describe the solution you'd like
Ability to pass the current path directly to the component, potentially via the SidebarComponent context as one possible solution.

Describe alternatives you've considered
Use react router within my nextjs app, I would rather not use client side routing.

Additional context
Happy to contribute if an agreed direction by the maintainers is proposed.
Ideally react-router should be opt in rather than having to opt out in my eyes.

@rluders
Copy link
Collaborator

rluders commented May 8, 2022

Hi @KaiSpencer,

Thanks for opening the issue. May I ask you for some more information?

  • What is the goal here?
  • What would the benefits be?

I don't get it, maybe @tulup-conner could help us here since he is heavily working on the Sidebar component. But, if you are talking to remove dependency libraries I'm always interested. 👍

@KaiSpencer
Copy link
Author

Hi @rluders

Sure thing, the sidebar requires react-router, it uses a hook to determine the url and the Link component to handle navigation. This isn't something that every react application wants by default. I think client side routing should be opt in for the component

In my scenario I use Nextjs with its own router and don't want to have to switch to client side routing just to use this component.

MUI may be a good starting point to take some hints how to handle supporting various routing libraries.
https://mui.com/material-ui/guides/routing/#main-content

@rluders
Copy link
Collaborator

rluders commented May 8, 2022

@KaiSpencer all right... I think that I got it., but please correct me if I'm wrong.

The overall idea is to allow the sidebar component to receive a "router adapter" to use internally to generate the links. So the user can easily replace it in order to use the router that best fits his application.

Something like this? It sounds like a good idea IMHO.

@rluders rluders added the 🚀 enhancement New feature or request label May 8, 2022
@KaiSpencer
Copy link
Author

@rluders Yeah, it could be as simple as optionally passing an override to the Link component. And rather than using react-router useLocation, use window.href.pathname instead for the current route.

@tulup-conner
Copy link
Collaborator

Yea I agree completely. It's already being used in another component so I added it thinking that was the intended way to go.

I didn't think about the fact that react-router in Next takes on the role of client side routing. That is definitely not desirable. I'm spoiled by Remix.

@rluders
Copy link
Collaborator

rluders commented May 9, 2022

OK. So, I guess that we agree that remove the react-router and to add the flexibility to use others routers librabries is something that we want.

@KaiSpencer if you want to go ahead and submit a PR implementing this feature we gonna be happy to review it and accept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants