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

Add new SPA template #8

Merged
merged 16 commits into from
Aug 10, 2017
Merged

Add new SPA template #8

merged 16 commits into from
Aug 10, 2017

Conversation

jvanbruegge
Copy link
Collaborator

@jvanbruegge jvanbruegge commented Aug 3, 2017

The current version is working, but there is still work left to do

@jvanbruegge
Copy link
Collaborator Author

cc @SteveALee

@SteveALee
Copy link
Member

I already updated the readme. Is more needed? I also tweaked the test but just noticed is broken due to a path error in import.

BTW - should we change the routes? I think / is good but other could be /page2 or /speaker

@jvanbruegge
Copy link
Collaborator Author

jvanbruegge commented Aug 3, 2017

I want to restructure the Readme a bit.

The tests were broken due to a path error, but coverage was not 100% any more, so I added two new tests.

Yes, I will change the route

@jvanbruegge jvanbruegge self-assigned this Aug 3, 2017
@SteveALee
Copy link
Member

One more thing for your list is to make sure create-cycle-app --flavor still works now repo changed

@SteveALee
Copy link
Member

Thanks for doing tests. I can cross of my list :)

@jvanbruegge
Copy link
Collaborator Author

The package name stays the same, that's not a problem

@SteveALee
Copy link
Member

SteveALee commented Aug 3, 2017 via email

@jvanbruegge
Copy link
Collaborator Author

A current version for trying out can be installed with

create-cycle-app test --flavor cycle-scripts-one-fits-all@beta

@jvanbruegge
Copy link
Collaborator Author

Coverage is now working for the whole codebase, not only the files that have tests.
Plus babel is not needed any more as a direct dependency (still indirect as the istanbul uses babel).
coverage

@jvanbruegge
Copy link
Collaborator Author

So, speaker coverage is now 100% too.
coverage2

@SteveALee
Copy link
Member

SteveALee commented Aug 4, 2017

Please update PR comment to:
"PR for cyclic-router sink type (cyclejs-community/cyclic-router#198)

I stuck to the current convention of using the form Stream<HistoryAction> in the using code. Do you want related a PR to sync this one?

@jvanbruegge
Copy link
Collaborator Author

Yes, then i will sqash&merge

@jvanbruegge
Copy link
Collaborator Author

@SteveALee testing with cyclic-router is quite painful, so I opened cyclejs-community/cyclic-router#200

@jvanbruegge jvanbruegge force-pushed the spa-continue branch 4 times, most recently from 7bf40c6 to 8e91a04 Compare August 9, 2017 11:23
@jvanbruegge jvanbruegge mentioned this pull request Aug 9, 2017
3 tasks
@jvanbruegge
Copy link
Collaborator Author

So, now I'm just waiting to get merge rights on @cycle/storage and this can be merged

@SteveALee
Copy link
Member

Wh00t

@jvanbruegge jvanbruegge mentioned this pull request Aug 10, 2017
@jvanbruegge jvanbruegge changed the base branch from master to 5.0 August 10, 2017 11:26
@jvanbruegge jvanbruegge merged commit 04ae9f8 into 5.0 Aug 10, 2017
@SteveALee
Copy link
Member

Yippee! I wonder how Github can give me notifications for releases? There aught to be a way

@jvanbruegge jvanbruegge deleted the spa-continue branch August 10, 2017 11:30
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.

None yet

2 participants