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

[WIP] experimental new structure #57

Merged
merged 3 commits into from
May 5, 2018
Merged

[WIP] experimental new structure #57

merged 3 commits into from
May 5, 2018

Conversation

Rich-Harris
Copy link
Member

@MiYogurt
Copy link

MiYogurt commented May 5, 2018

look like not working , because you change the init method arguments。

i did not understand you pseudo-code,i did not known where app from in sapper and selectRoute method ,i found selectRoute method in runtime.ts but not exported . i will expect more simple and clear sample code。

tks~ 🤗

@Rich-Harris
Copy link
Member Author

It's not working because the new version of Sapper that it depends on hasn't been released yet.

@Rich-Harris Rich-Harris merged commit 9d6dd4a into master May 5, 2018
@Rich-Harris Rich-Harris deleted the layout branch May 5, 2018 14:11
@@ -6,19 +6,21 @@
"dev": "sapper dev",
"build": "sapper build",
"export": "sapper export",
"start": "sapper start",
"start": "node build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? shouldn't this be npm build?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's correct, yeah — as of 0.11, Sapper creates a launcher file which defaults to build/index.js. So to run the app you just to node build

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably explains my problems w/ trying to build when upgrading Sapper. ;) Need to upgrade sapper-template too...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sapper start still works

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it all working.

@@ -0,0 +1,31 @@
<Nav page={
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rich-Harris why the intensive props.path logic here? This seems very manifest-y, which goes against the grain of the automated path-finding you seem to support? What exactly is the function of these page props?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, do I have to add to this for every page I add?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass props.path down to <Nav> instead (in fact, I prefer that — will change it)...

<Nav path={props.path}/>

...but yes, you have to explicitly pass stuff down, just like you used to have to explicitly pass down a page property via the <Layout> component in the previous template. How else will the nav bar know which option to mark as active?

@arxpoetica
Copy link
Member

That's much better, thx.

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