[WIP] Add an example with page loading indicator #250
Closed
+178
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 14, 2016
|
@rauchg I think we could pass the But I hope this is enough for now. |
nkzawa
reviewed
Nov 14, 2016
| const { | ||
| onStart = () => null, | ||
| onComplete = () => null, | ||
| onError = () => null |
There was a problem hiding this comment.
👍 I didn't know this is possible
There was a problem hiding this comment.
Ha ha. ES6 is pretty mysterious.
There was a problem hiding this comment.
You can do this also:
onStart: theNewNameForOnStart = () => null
|
This is a beautiful example, specially because it shows integration with a 3rd party (even non-react!) library. I would like to add That's the format we're going with for all examples/, since right now they're kinda cryptic. |
|
@rauchg sure. Will do it. |
|
@nkzawa I hope we could take this and land this on the next release. |
rauchg
reviewed
Nov 18, 2016
| <div style={{ marginBottom: 20 }}> | ||
| <Head> | ||
| {/* Import CSS for nprogress */} | ||
| <link rel='stylesheet' type='text/css' href='http://ricostacruz.com/nprogress/nprogress.css' /> |
There was a problem hiding this comment.
maybe make a copy of this into /static instead? Since this is http and we don't want to spam his site
rauchg
reviewed
Nov 18, 2016
| "version": "0.0.0", | ||
| "description": "", | ||
| "scripts": { | ||
| "dev": "../../dist/bin/next" |
There was a problem hiding this comment.
This requires a top-level npm install, which is not clarified in the instructions. I think it's best if we include maybe "next": "*" in the deps?
There was a problem hiding this comment.
Okay that makes sense. I will right another script to play with current source code.
rauchg
requested changes
Nov 18, 2016
|
I noticed loading indicator would be required for history back ( |
|
Good point @nkzawa. Let's address it |
|
Good catch @nkzawa :) Let's see. |
|
Since we need to support popstate events, I think it's better to remove onStart, onStop events from the Link and expose a similar API to the singleton Router. Just like Router.go() |
|
@rauchg I changed the example as you suggested. |
arunoda
changed the title
|
@arunoda |
|
Closing this in favor of #511 |
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles
Commit: 0410a43 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Check this:
In order to do this, I have to introduce two new events to the ``. They are:
Why not using onClick instead of onStart?
onClick gets called even any clicks including external navigations. That's why we need this event.