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

Layout for persistent components #2440

Closed
wants to merge 3 commits into from
Closed

Layout for persistent components #2440

wants to merge 3 commits into from

Conversation

Xerios
Copy link

@Xerios Xerios commented Jul 2, 2017

A quick working implementation of an optional top-most component that wraps pages, in other words this is the missing layout system.

I had some trouble figuring out how next handled default files, so I did my best to make it work with minimum amount of code change or impact on existing projects.

The way it works is simple, if there's a _layout.js file inside the pages folder, it'll use that to wrap Component. Othwerwise, it uses the default Layout ( which is basically a div wrapper ).

Related to this: #88

@timneutkens
Copy link
Member

This actually won't fix #88 since ReactDOM.render is still called on page changes. cc @arunoda

@arunoda
Copy link
Contributor

arunoda commented Jul 3, 2017

This is an interesting approach and I was also looking for something like this.
Anyway, let's discuss more about this after 3.0

@timneutkens timneutkens added this to the After 3.0 milestone Jul 3, 2017
@Xerios
Copy link
Author

Xerios commented Jul 3, 2017

@timneutkens True that it won't fix the re-render ( I was planning to look into that later ), but it does fix my issues with toast and transition plugins since the layout components are not page-bound.

@mattfysh
Copy link

mattfysh commented Jul 7, 2017

@timneutkens @arunoda are there any other issues/pull requests I can follow re: not using ReactDOM.render between routes? I can see the benefits in terms of animations and maintaining state for common elements, but other problems I'm seeing in the examples:

  1. assuming the common elements won't be re-instantiated between routes e.g. apollo - where ApolloProvider's constructor is triggered on each route, which forwards a call to client.initStore. The only reason this is working is because apollo-client doesn't throw an error if you call initStore on an already instantiated client, and ignores the call - other libraries can be less forgiving
  2. in contrast, with-react-ga only works because of the re-instantiation of all page elements, using the constructor function of the top-level element to trigger a pageview

It would be great if the next.js router behaved a little more like the other routers which use React Reconciliation to switch between routes, leaving common elements in-tact between routes. Hope that makes sense, let me know if I can help in any way.

@tannerlinsley
Copy link

I have been back and forth between just about every static site generator out there and can't seem to find one that top's Next.js's ease of use and API. This feature however is pretty crucial to a decision to start building with Next.js though. This particular implementation seems to fix a longstanding problem I've seen with almost all SS react implementations: thrashing react component lifecycle methods. Obviously this doesn't fix the ReactDOM.render problem, but it does fix the lifecycle thrashing for layout-level components. I've been through the code and also tested locally several time and this seems to be the missing link for me.

@OutThisLife
Copy link
Contributor

Does #2613 do this w/in the current version?

@rileybracken
Copy link

@arunoda can we breath some life into this PR now that 3.0 is out?

@arunoda
Copy link
Contributor

arunoda commented Aug 31, 2017

@rileybracken sure.
I'm starting to take some backlog PRs and fixing issues.

@tz5514
Copy link

tz5514 commented Oct 6, 2017

Wish this feature will be released with 4.0 stable~~

@juhaelee
Copy link

@timneutkens is there a reason this was closed?

@timneutkens
Copy link
Member

@juhalee most likely Cause it was pointing at the v3-beta branch. I didn't manually close it

@lock
Copy link

lock bot commented May 10, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
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.

None yet

9 participants