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

feat: Added support for declaring router layout #245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CatchABus
Copy link
Contributor

This PR adds support for layout per Router.

@ItalyPaleAle
Copy link
Owner

Thanks for this PR. What is the problem you're trying to solve with this? What does it allow doing that you can't otherwise do?

@CatchABus
Copy link
Contributor Author

CatchABus commented Nov 10, 2021

Thanks for this PR. What is the problem you're trying to solve with this? What does it allow doing that you can't otherwise do?

Here is a concept this would help.
I switch between 2 layouts depending on authentication state:

<svelte:component this={$layout}>
  <Router {routes} on:conditionsFailed={conditionsFailed}/>
</svelte:component>

Even though the above does its work, it affects the lifecycle of Router instance causing it to get destroyed and mounted once more if layout is changed.
Of course, I could use 2 Routers in such a case or include layout inside page component itself but then it goes deeper.
For example, I would want to hide both my layout and page, in order to show a spinner indicator or have a great number of layouts.

@CatchABus
Copy link
Contributor Author

CatchABus commented Nov 16, 2021

In general, it would be flexible to add components between Router and view component.
Adding parents inside view component itself ruins UI and UX, since import() will have a first time delay and app will seem blank.
If you take a look at svelte-router-spa package, author splits main component into Router and Route.
The benefit of this is you can add route component inside Router as a child or grand child or even grand grand child.

@CatchABus CatchABus closed this Jan 7, 2022
@davidhaley
Copy link

@CatchABus, is there a reason that you closed this before it was merged?

@CatchABus
Copy link
Contributor Author

@CatchABus, is there a reason that you closed this before it was merged?

Hello. Not sure, I probably thought it might not be accepted as a new feature.
I still think it'd be helpful to have it though.

@CatchABus CatchABus reopened this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants