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

Update grunt-csscomb #13873

Merged
merged 2 commits into from
Jul 22, 2014
Merged

Update grunt-csscomb #13873

merged 2 commits into from
Jul 22, 2014

Conversation

XhmikosR
Copy link
Member

This is a WIP until csscomb has the remaining issues sorted on their side, thus the output CSS should be the same then.

For reference csscomb/csscomb.js#232

@XhmikosR XhmikosR added the grunt label Jun 19, 2014
@XhmikosR XhmikosR added this to the v3.2.1 milestone Jun 19, 2014
@XhmikosR XhmikosR self-assigned this Jun 19, 2014
@XhmikosR
Copy link
Member Author

@tonyganch: you can see here the changes in comments.

@mdo
Copy link
Member

mdo commented Jul 13, 2014

What's the status on this? Can't say I'm enthusiastic about moving inline comments to the next line. Also, tons of this conflicts with what I'm working on for v4, so maybe we just punt it until that kicks off publicly?

@tonyganch
Copy link

I've fixed behaviour of comments.
I've also cloned this branch and run grunt with new CSScomb to see the new diff:
tonyganch/bootstrap@8d348ed
I'm not sure why .map files are modified, maybe I do something differently.
But you can see that your inline comments are safe now.

There is also one line I can do nothing about:
tonyganch/bootstrap@8d348ed#diff-4e5779e4dffa63415fd5bda19e74ff90L103
The diff is caused by space-after-opening-brace: "\n" and I'm pretty sure that's a correct way for this option to work.

There will be no more beta releases of CSScomb, so these "comment" changes will be published some time later with a stable release.

@XhmikosR
Copy link
Member Author

I think that looks good @tonyganch, thanks!

I will wait for a new npm package and then I'll update the PR and merge.

@XhmikosR
Copy link
Member Author

BTW the map files change must be because of clean-css version difference but nothing to worry about.

@cvrebert
Copy link
Collaborator

We should check whether the new CSSComb v3.0.0 addresses our problem.

@XhmikosR
Copy link
Member Author

@tonyganch
Copy link

Let me know if the problem remains, please

@XhmikosR
Copy link
Member Author

@tonyganch: the npm package seems the same (3.0.0-1). Did you forget to publish a new one?

EDIT: nvm, I confused the grunt package with csscomb itself 👊

@tonyganch
Copy link

@XhmikosR, Version 3.0.0 last updated 13 minutes ago

-> % npm i csscomb
csscomb@3.0.0 ../node_modules/csscomb
├── commander@2.0.0
├── vow@0.4.4
└── csscomb-core@2.0.1 (minimatch@0.2.12, vow-fs@0.3.2)

@XhmikosR
Copy link
Member Author

Yeah, see my edit above :/

Everything looks good now, I'll merge it when I'm back home since I don't want to do things in a hurry :)

"space-after-colon": 1,
"space-after-combinator": 1,
"space-before-selector-delimiter": 0,
"space-after-declaration": "\n",

Choose a reason for hiding this comment

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

This line should be replaced with "space-between-declarations": "\n"

XhmikosR added a commit that referenced this pull request Jul 22, 2014
@XhmikosR XhmikosR merged commit 8b6d396 into master Jul 22, 2014
@XhmikosR XhmikosR deleted the grunt-csscomb branch July 22, 2014 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants