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

Use postcss-preset-env instead of cssnext #239 #240

Merged
merged 1 commit into from May 29, 2018

Conversation

ur300
Copy link

@ur300 ur300 commented May 27, 2018

postcss-preset-env added to the main page instead of cssnext

@ai
Copy link
Member

ai commented May 27, 2018

@ur300 it is always better to mention main maintainer in New pull request

/cc @marcustisater

@marcustisater
Copy link
Member

Thanks for PR @ur300 👍Remember to reference the issue next time. Let me look through code and copy.

@marcustisater
Copy link
Member

marcustisater commented May 27, 2018

Whops sorry my fault @ur300. You did reference it in the title 😄

Code looks good but I think the SVG should have a set height and width with correct viewbox like https://github.com/postcss/postcss.org/blob/master/web_modules/InANutshell/css-modules.svg has. Would you mind fixing that? That would be awesome!

On the other hand I think the order should be like before, so

  1. Autoprefixer.
  2. Preset Env
  3. CSS Modules
  4. Stylelint
  5. LostGrid

@ai @jonathantneal is the copy ok? See attacthed screenshot.

order

@@ -7,6 +7,7 @@

.logo {
margin-right: 1rem;
max-width: 80px;
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. Set width and height through SVG.

@ai
Copy link
Member

ai commented May 27, 2018

Yeap. Let's more Preset Env to second position.

And let's remove pv- from media query name to make it simpler.

@@ -127,11 +128,12 @@

.infoAlt {
composes: info;
margin-left: 1.5rem;
margin-left: 0.5rem;
Copy link
Member

Choose a reason for hiding this comment

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

I think this looks a bit too tight.

@jonathantneal
Copy link
Member

This all seems good to me.

@ur300
Copy link
Author

ur300 commented May 27, 2018

hi @marcustisater, thanks for review. all the fixes are done - svg size, margin, order of features, media query name.

@ai
Copy link
Member

ai commented May 28, 2018

@ur300 task is marked as solved by you https://cultofmartians.com/tasks/postcss-preset-env-postcss.html

@jonathantneal
Copy link
Member

@ai, sweet, is this being merged today?

@ai
Copy link
Member

ai commented May 28, 2018

We waiting for @marcustisater review and deploy

@marcustisater
Copy link
Member

SVG viewbox is still odd, but I can fix it later. LGTM

@marcustisater marcustisater merged commit 3d2d98f into postcss:master May 29, 2018
@marcustisater
Copy link
Member

marcustisater commented May 29, 2018

@ai @jonathantneal bah, something is wrong with deploy 😢 I think it has something to do with the node env or when we switched over to HTTPS. Let me check tonight. Sorry for troubles! I will let you know when it's fixed.

@marcustisater
Copy link
Member

@ai @jonathantneal I deployed from local env, should be up live now 😄 I will add new issue for auto deploy trouble. 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants