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

[docs] Add a next.js demo with hooks #13920

Merged
merged 2 commits into from
Dec 19, 2018

Conversation

oliviertassinari
Copy link
Member

Closes #13773

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Dec 16, 2018
@virzak
Copy link
Contributor

virzak commented Dec 17, 2018

Thanks for that!!!

What's the significance of

// Use the same helper as Babel to avoid bundle bloat.
import { install } from '@material-ui/styles';

install();

Tried to remove import '../src/bootstrap'; and didn't see anything different.

Also, I converted this example into typescript. Let me know if you would like a PR.

@oliviertassinari
Copy link
Member Author

@virzak The install() call makes the core components use the new styles implementation. We can add a comment about it. We have a create-react-app example with TypeScript. Is that enough?

@virzak
Copy link
Contributor

virzak commented Dec 17, 2018

I can only speak for myself, but any time I get a sample of a base library, I convert it to typescript. There is a lot of code in next.js (most notably _document.tsx and _app.tsx) which is not covered by the create-react-app example. Also I just find that typescript forces you to produce cleaner code.

Here is the repo: https://github.com/virzak/material-ui-nextjs-hooks-typescript

Added:

Issues:

  • Not entirely sure what to do in App constructor
  • pageContext needs to be cast at PageContext for some reason, even though it is never undefined. Not sure if there is a good way to avoid it.

@virzak
Copy link
Contributor

virzak commented Dec 17, 2018

So what you're saying is import '../src/bootstrap'; is not necessarily needed? Does it make a difference for the users of the library if core components use the old implementation?

Also does the import order matter? ts-lint wants all imports sorted alphabetically.

@oliviertassinari
Copy link
Member Author

@eps1lon What do you think of a next.js example using TypeScript?

@virzak The import order matters. Yes, it's important to call it with server-side rendering. It's allowing the collection of the CSS and might prevent a classnames clash issue.

@eps1lon
Copy link
Member

eps1lon commented Dec 19, 2018

We could merge the example from @virzak into the main repo no objection here if he wants to work on that. Glanced over the repo quickly and it looked fine to me with some minor issues.

@oliviertassinari oliviertassinari merged commit 9445b61 into mui:master Dec 19, 2018
@oliviertassinari oliviertassinari deleted the docs-next-hook branch December 19, 2018 18:59
@oliviertassinari
Copy link
Member Author

@virzak It looks like we have a green light for this TypeScript demo :) 💚 .

@@ -0,0 +1,40 @@
/* eslint-disable jsx-a11y/anchor-is-valid */
import '../src/bootstrap';
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary from my understanding since we import this already in _app.js and _app.js is executed on client and server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Console log the execution order between the client and the server, it's different. We need to call it first. At least, until v4 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants