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

bump(postcss): update postcss version to 8.0.0 #37

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

nehapatwardhan6490
Copy link

@nehapatwardhan6490 nehapatwardhan6490 commented Oct 28, 2020

What Changed

Upgraded version of postcss to 8.0.0

Why

It's the latest version and gives improved performance
Todo:

@tylerkrupicka
Copy link
Contributor

You're probably going to have to change our node version to 12 in the circle CI file for build to pass.

Copy link
Contributor

@tylerkrupicka tylerkrupicka left a comment

Choose a reason for hiding this comment

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

Stepped through the changes while looking at the migration guide. Great work so far! There were a couple of improvements in the migration guide that we should probably update, but otherwise it looks great.


root.walkRules(rule => {
const themedDeclarations: postcss.Declaration[] = [];
root.walkRules(walkRule => {
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Looking at the migration guide, I think we should remove the root.walk and use Rules api instead.

@@ -458,30 +464,30 @@ const modernTheme = (
mergedSingleThemeConfig && hasDarkMode(mergedSingleThemeConfig);

// 1. Walk each declaration and replace theme vars with CSS vars
root.walkRules(rule => {
rule.selector = replaceThemeRoot(rule.selector);
root.walkRules(walkRule => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about switching walkRules to Rules if possible.

@@ -622,4 +628,4 @@ const themeFile = (options: PostcssThemeOptions = {}) => (
}
};

export default postcss.plugin('postcss-themed', themeFile);
export default themeFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Looking at the migration guide, I think there might be a couple of recommended attributes we should include like postCSSPlugin and Once.

@hipstersmoothie hipstersmoothie added the major Increment the major version when merged label Oct 30, 2020
@tylerkrupicka
Copy link
Contributor

Hey @nehapatwardhan6490, I just merged #35 which refactored some of the code into different files. It's going to cause some conflicts here--really sorry about that. Let me know if you need any help merging the changes.

@tylerkrupicka
Copy link
Contributor

@nehapatwardhan6490 do you need any help with this?

@nehapatwardhan6490
Copy link
Author

Hey, sorry didn't get a chance to look in to this. WI check it out in next few days and get back to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest major Increment the major version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants