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.

Sign up for GitHub

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

Jump to bottom

Upgrade next #218

Merged
merged 3 commits into from
Apr 29, 2020
Merged

Upgrade next #218

merged 3 commits into from
Apr 29, 2020

Conversation

Joozty
Copy link
Member

@Joozty Joozty commented Mar 30, 2020 •
edited
Loading

In the first commit, I removed the deprecated getInitialProps method and did appropriate changes. It was pretty straightforward. Nonetheless, the second commit is more interesting.

I decided to switch to the new postcss default support in NextJS. There were some breaking changes. I had to rename all css files to module.css and move variables to classes. :root selector is not supported. There is a bug reported so when it gets fixed we can move them back. (Maybe it is even better as is now. I find it as a correct solution).

The reason why I decided to switch it was mainly because hot reaload didn't work very well and also we had some problems with css from design repo. This has been all fixed now.

We will need to postpone this. CSS in dev mode is imported incorrectly. See vercel/next.js#10148

@vercel
Copy link

vercel bot commented Mar 30, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oacore/dashboard/2i9fjcko1
✅ Preview: https://dashboard-git-upgrade-next.oacore.now.sh

@Joozty Joozty self-assigned this Mar 30, 2020
@Joozty Joozty added the ✨ enhancement New feature or request label Mar 30, 2020
@Joozty Joozty marked this pull request as ready for review March 30, 2020 11:01
viktor-yakubiv
viktor-yakubiv reviewed Mar 31, 2020
View reviewed changes
Copy link
Member

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

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

Will review the rest later, sorry.

postcss.config.js Show resolved Hide resolved
design/card/index.module.css Outdated Show resolved Hide resolved
viktor-yakubiv
viktor-yakubiv requested changes Mar 31, 2020
View reviewed changes
Copy link
Member

@viktor-yakubiv viktor-yakubiv left a comment

Choose a reason for hiding this comment

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

Please, remove breaking changes with :root selector replacing it with html. Apart from that it looks good to me.

Why are you obliged to rename all the CSS file with .module suffix?

How the design library is supported now?

next.config.js Show resolved Hide resolved
pages/_app/index.module.css Outdated Show resolved Hide resolved
@Joozty
Copy link
Member Author

Joozty commented Mar 31, 2020

Why are you obliged to rename all the CSS file with .module suffix?

It's how nextjs distinguish postcss files from regular ones. I don't like it but it is what it is.

How the design library is supported now?

It's guaranteed that design css files will be imported before any local ones.

@viktor-yakubiv viktor-yakubiv self-requested a review March 31, 2020 15:16
@Joozty Joozty mentioned this pull request Apr 1, 2020
Closed
@Joozty Joozty marked this pull request as draft April 17, 2020 09:29
@Sebastp
Copy link

Sebastp commented Apr 17, 2020

any updates?

@Joozty
Copy link
Member Author

Joozty commented Apr 17, 2020

@Sebastp Not yet. We are still waiting for fixing this bug vercel/next.js#11946

@Joozty
Copy link
Member Author

Joozty commented Apr 22, 2020

@viktor-yakubiv So the bug regarding CSS order has been just fixed. We can use the canary version and merge or wait for version 9.3.6. What do you prefer?

@viktor-yakubiv
Copy link
Member

If waiting drains you because of unclosed task, let's merge. Otherwise let's wait.

@vercel vercel bot temporarily deployed to Preview April 29, 2020 07:00 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 07:00 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 07:04 Inactive
@Joozty Joozty marked this pull request as ready for review April 29, 2020 07:04
@Joozty Joozty merged commit e51ed0b into master Apr 29, 2020
@Joozty Joozty deleted the upgrade-next branch April 29, 2020 07:09
@viktor-yakubiv
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viktor-yakubiv viktor-yakubiv viktor-yakubiv approved these changes

Assignees

@Joozty Joozty

Labels
✨ enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joozty @Sebastp @viktor-yakubiv