search

sveltejs / sapper-template

Starter template for Sapper apps
703 stars 214 forks source link

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

Closed thebells1111 closed 3 years ago

thebells1111 commented 4 years ago

in anticipation of https://github.com/sveltejs/sapper/issues/824

maxmilton commented 4 years ago

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

Conduitry commented 4 years ago

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 commented 4 years ago

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

thebells1111 commented 4 years ago

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 commented 4 years ago

@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 commented 3 years ago

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