-
Notifications
You must be signed in to change notification settings - Fork 1
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
Custom render page function & with-next-layouts example #2
Conversation
a1fc015
to
8ca8649
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tz5514 Thanks for trying this approach!
I have some minor naming suggestions. More importantly, I tend to think the raw props should be passed to renderPage
.
lib/app.js
Outdated
@@ -72,9 +72,14 @@ class Container extends Component { | |||
|
|||
render () { | |||
const { Component, props, url } = this.props | |||
const component = (typeof Component.renderPage === 'function') ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick, could we name this page
instead of component
?
lib/app.js
Outdated
@@ -72,9 +72,14 @@ class Container extends Component { | |||
|
|||
render () { | |||
const { Component, props, url } = this.props | |||
const component = (typeof Component.renderPage === 'function') ? ( | |||
Component.renderPage({ Component, props, url }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this just:
Component.renderPage(this.props)
In other word, why do most of this.props
get packed into props
? It seems like to me that renderPage
should get the same props as Component
gets + the Component
itself, which is just the original this.props
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
200% agree! 🔥🔥🔥
@@ -0,0 +1,81 @@ | |||
import React from 'react' | |||
const defualtOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops 😮
8ca8649
to
713d81b
Compare
@jdeal fixed them all. thanks for suggestions 😄 ! |
@tz5514 Let's PR it to the upstream, so this get's reviewed by the core team. |
@frol got it! |
713d81b
to
53db6d5
Compare
@tz5514 Do you need help with anything? |
@frol I'm writing for new PR right now ha ha. |
Yeah, I would close both vercel#3461 and vercel#3471 and reference the new PR from there. |
@frol The new PR is here. vercel#3552 |
53db6d5
to
8649e43
Compare
* Add a nerv example. * Fix for indentation/style * Fix for name
* Use indexOf instead of startsWith This fixes an IE11 regression, see vercel#3755 * Lint the code
@tz5514 Oh my, please, do rebase next time instead of merge. |
@frol oops, my bad |
* Update readme for Next 5 * remove specific version mention
* Remove deprected use of apollo-client-preset, and refactor Changes * Remove deprected use of apollo-client-preset in favor of apollo-boost * Refactor for usage of react-apollo@2.1 * Use standard Just ran standard --fix
* New example: with-now-env * updated the example with-now-env
* Fix vercel#3900 return 404 on asset hash mismatch in prod * Make INVALID_BUILD_ID return 404
* Better support React 16.3.0 mridgway/hoist-non-react-statics#43 * Lock version
* Fix serve command From the Firebase docs, you must use --only in order to run the local function emulator. See https://firebase.google.com/docs/functions/local-emulator * Add production env for firebase serve Doesn't work without this * Update text as suggested by @jthegedus
Makes sure highlight.js/styles/dark.css doesn’t get caught by externals.
…cel#3956) * Add router method to inject code before popstate events * Default _beforePopState, return true * Fix link in README * Re-order `if` statements per feedback
* Allow BUILD_ID to be set in the environment This makes multi server deploys with Capistrano possible. * next.config.js generateBuildId support enable customising the build id via config * Provide default for generateBuildId * This is not used
Upgrade Babel 6 to Babel 7 (major)
* Add support for exportPathMap in development * Add comment about what it does
3d6601f
to
4cc09fe
Compare
No description provided.