Skip to content
This repository was archived by the owner on Oct 13, 2022. It is now read-only.

use $page from stores in lieu of segment prop #230

Closed
wants to merge 2 commits into from

Conversation

thebells1111
Copy link

@thebells1111 thebells1111 commented Jun 2, 2020

in anticipation of sveltejs/sapper#824

@maxmilton
Copy link

Should this PR also remove segment from src/routes/_layout.svelte?

@Conduitry
Copy link
Member

Yeah, it probably should. I'm also not sure how I feel about continuing to use segment as a reactive declaration name here. If there's another way this could be written that does invoke the concept of 'segment' at all, I would probably be more comfortable with that.

@thebells1111
Copy link
Author

Do you mean using something like currentPage as the variable name in lieu of segment?

@thebells1111
Copy link
Author

If segment is removed as a prop for the Nav component in src/routes/_layout.svelte, this warning is displayed.

Layout has unused export property 'segment'

If export let segment is left undeclared in src/routes/_layout.svelte, this warning is displayed in the browser console.

<Layout> was created with unknown prop 'segment'

@benmccann
Copy link
Member

@thebells1111 can you go ahead and remove it from _layout? That warning will go away when we remove the property from the component definition in Sapper. But maybe we should merge this PR at the same time we make that change to avoid users seeing the warning

As far as naming, I think it's fine to call this segment. That's the typical term for the piece between two slashes in a URL path. And it might also help people who need to migrate understand this as the alternative. If we want a different name though then dir could be another option

@benmccann
Copy link
Member

Thanks for this. Sapper's in maintenance mode now, so I'm not sure that sveltejs/sapper#824 will be changes, which means this PR wouldn't be applicable

@benmccann benmccann closed this Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants