-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
feat(create-next-app): update install process, add support for --tailwind
options
#24709
Conversation
This unused variable in a template was originally removed in order to prevent ESLint from throwing.
Also, for the new Tailwind templates, most of the styles were redone with Tailwind classes, and a Would recommend adjusting the styles in the default templates as well to get rid of this bug but that would be a separate PR. |
6f6f51d
to
c62a0dc
Compare
--typescript, --tailwind
options --tailwind
options
Fixes #27770. Projects initialized with `create-next-app` should not only be totally functional in terms of styling, but ideally there would be as minimal CSS as possible to serve as a "jumping off point" for users (related: #24709, where I initially proposed this fix). However, minor issues with the specific styling approaches in the template created by `create-next-app` break the page for small viewports. This is caused by an improper use of `display: flex` in conjunction with `min-height: 100vh` on the `.container` class, and I imagine it places a drag on the initial user experience due to the fact that every user who creates a project with `create-next-app` must *independently fix* this viewport sizing issue themselves. For example, the top of the viewport on a small display (here, iPhone SE): ![image](https://user-images.githubusercontent.com/1657236/129430078-229d5fab-b719-458c-8a94-10fb8be3490d.png) Notice the "Welcome to Next.js" message is missing: Its container is larger than the viewport, and is set to be a flex column with `justify-center`—so we are staring at the center of the container. To demonstrate better, here is the full page render for before and after this PR: ![image](https://user-images.githubusercontent.com/1657236/129430409-52134198-651a-4cf8-915d-68b699febd77.png) This PR adjusts the styling to fix this issue, and also other styling issues on small screens, like grid width issues causing text to overflow (and the footer padding as seen above): ![image](https://user-images.githubusercontent.com/1657236/129430114-1dda7674-3b02-45d4-a4b3-37fc5053c6c4.png) After these changes, and minor padding tweaks, this is the top of the viewport on an iPhone X (full render above): ![image](https://user-images.githubusercontent.com/1657236/129430224-1991c1a6-8c7e-4246-b4a5-44919fb850c6.png) And on an iPhone SE: ![image](https://user-images.githubusercontent.com/1657236/129430259-4408c52f-6fc6-4f22-9cc6-bbdbbe8d7a1a.png)
Fixes vercel#27770. Projects initialized with `create-next-app` should not only be totally functional in terms of styling, but ideally there would be as minimal CSS as possible to serve as a "jumping off point" for users (related: vercel#24709, where I initially proposed this fix). However, minor issues with the specific styling approaches in the template created by `create-next-app` break the page for small viewports. This is caused by an improper use of `display: flex` in conjunction with `min-height: 100vh` on the `.container` class, and I imagine it places a drag on the initial user experience due to the fact that every user who creates a project with `create-next-app` must *independently fix* this viewport sizing issue themselves. For example, the top of the viewport on a small display (here, iPhone SE): ![image](https://user-images.githubusercontent.com/1657236/129430078-229d5fab-b719-458c-8a94-10fb8be3490d.png) Notice the "Welcome to Next.js" message is missing: Its container is larger than the viewport, and is set to be a flex column with `justify-center`—so we are staring at the center of the container. To demonstrate better, here is the full page render for before and after this PR: ![image](https://user-images.githubusercontent.com/1657236/129430409-52134198-651a-4cf8-915d-68b699febd77.png) This PR adjusts the styling to fix this issue, and also other styling issues on small screens, like grid width issues causing text to overflow (and the footer padding as seen above): ![image](https://user-images.githubusercontent.com/1657236/129430114-1dda7674-3b02-45d4-a4b3-37fc5053c6c4.png) After these changes, and minor padding tweaks, this is the top of the viewport on an iPhone X (full render above): ![image](https://user-images.githubusercontent.com/1657236/129430224-1991c1a6-8c7e-4246-b4a5-44919fb850c6.png) And on an iPhone SE: ![image](https://user-images.githubusercontent.com/1657236/129430259-4408c52f-6fc6-4f22-9cc6-bbdbbe8d7a1a.png)
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.
Hi, thanks for the PR! Seems this is a bit stale now so I'm gonna close it for now, also I'm not sure we want to add a --tailwind
flag as it's equivalent to --example=tailwind
but maybe the alias does make sense. We can investigate this further on an idea discussion though!
Forked from my other PR, completes the rapid-fire development loop of Next + TypeScript + Tailwind by allowing for
--typescript --tailwind
. This involved refactoring the template logic significantly, but it is much more well-documented and easier to extend now, if there was ever an interest in adding future options.Sorry for the two rapid-fire PRs (which weren't asked for), I just wanted this functionality and know it is widely requested. This was originally not such a large rewrite, but naturally getting things to work properly involved adjusting a lot of the surrounding logic. I understand this code may not get used, but wanted to contribute it back anyway. Genuinely very sorry for how much there is to review here, but I tried to keep it well-documented.
Updates to install process
Support for
devDependencies
Support for devDeps during install was added in the TypeScript PR (#24655) since the use case implies it. The vast majority of the install logic was refactored and reorganized in addition to this change.
By default,
react
andreact-dom
are dependencies, andnext
is a devDepenedency, but a case could be added for any arbitrary option to add dependencies (or template installs, as seen below). For instance,tailwind: true
will imply Tailwind and PostCSS devDeps:Support for TS templates
The separate logic for installing from an example repo and installing from templates was pulled out of
create-app.ts
and put into separate files to make the parent function more legible and help with refactoring. Templates are now stored in the formtemplates/{ts|js}/{template}
, and the install process for a template (helpers/install/from-template.ts
) basically has two steps (this has not changed, just is more clear and self-evident in the refactored code):initialTemplate
, which is"default"
by default, i.e.templates/ts/default
ortemplates/js/default
.NextCreateOptions
, i.e.{ tailwind: true }
will imply additional template installs fromtemplates/{ts|js}/tailwind
.The logic for additional template installs is handled in a switch/case inside
helpers/install/from-template.ts
. This same pattern is also observed in the dependencies/devDependencies staging, as seen above.Demo
Repo created with
create-next-app --typescript --tailwind
:https://github.com/ctjlewis/next-typescript-tailwind
Deployed at:
https://next-typescript-tailwind-hwv2xe88o-ctjlewis.vercel.app/
Footnotes
The diff for
helpers/install.ts
is weird because it was moved toinstall.ts
inside aninstall/
directory, i.e. athelpers/install/index.ts
. It was also heavily refactored (iteratively) and is, IMO, much more manageable now as well.