csscomb / csscomb.js

CSS coding style formatter
http://csscomb.com/
MIT License
3.29k stars 457 forks source link

Lines between rulesets #436

Closed AoDev closed 8 years ago

AoDev commented 8 years ago

It's about continuing the work of @ahumphreys87 here: https://github.com/csscomb/csscomb.js/pull/405

Helping to solve the lines-between-rulesets

AoDev commented 8 years ago

@tonyganch @ahumphreys87 Here is a PR continuing this feature. Let me know if there is something missing. I just added tests with 2 lines.

On the other hand I saw that in the updated docs, it is said that someone can use "\n\n" but in the code it says that it can only use numbers. What should it be?

ahumphreys87 commented 8 years ago

@aodev thanks for picking this up. Yup that's my bad, we decided just to use numbers but I haven't updated the docs yet

On Thursday, 26 November 2015, AoDev notifications@github.com wrote:

@tonyganch https://github.com/tonyganch @ahumphreys87 https://github.com/ahumphreys87 Here is a PR continuing this feature. Let me know if there is something missing. I just added tests with 2 lines.

On the other hand I saw that in the updated docs, it is said that someone can use "\n\n" but in the code it says that it can only use numbers. What should it be?

— Reply to this email directly or view it on GitHub https://github.com/csscomb/csscomb.js/pull/436#issuecomment-159839619.

AoDev commented 8 years ago

Ok thanks, i'll update de docs. :)

ahumphreys87 commented 8 years ago

great work @aodev thanks so much for picking this up.

1 more thing that @tonyganch picked up on was there is nothing for SASS, what are your thoughts on this? Does Sass need this as well?

AoDev commented 8 years ago

@ahumphreys87 I would think that all options might be interesting for any preprocessor / syntax. But I am a "less guy" and do not know really. If requested later, it can be added later but if it's a must have to merge the PR then I will look into it.

ravasthi commented 8 years ago

@ahumphreys87, @AoDev, thanks for all your hard work on this. FWIW, I feel like merging sooner would be better, so the MVP can get to the users for in-the-wild testing faster, and support for Sass can get added on top later on.

AoDev commented 8 years ago

@ravasthi I am also for fast iterations.

AoDev commented 8 years ago

@tonyganch is there something missing to merge this ?

malcomio commented 8 years ago

sounds like this should fix #210 (or make it obsolete)

vincentaudebert commented 8 years ago

Any idea when it will be merged? Have same issue than #210 and indeed it would fix it I think.

timoxley commented 8 years ago

cc @sodatea

haoqunjiang commented 8 years ago

Will review it this week :)

ahumphreys87 commented 8 years ago

I don't think the tests added here cater for this scenario https://github.com/csscomb/csscomb.js/pull/405#issuecomment-159568945, specifically 3 rulesets on the same line.

At least it's not clear where this case is tested

ravasthi commented 8 years ago

Hi, I could use a little help. I'm trying to get set up locally so that I can run tests, and I've run into a few problems:

  1. When I first try to install dependencies, it fails because npm is trying to run the post install script for csscomb-core before its dependencies are installed. I got around that by first doing npm install --ignore-scripts to get the dependencies, then npm install again to run the scripts.
  2. When I try to run npm test after that, I'm getting the following error:
---------------
 Running Mocha
---------------

module.js:341
    throw err;
    ^

Error: Cannot find module 'csscomb-core'
    at Function.Module._resolveFilename (module.js:339:15)
    at Function.Module._load (module.js:290:25)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at Object.<anonymous> (/Users/ravasthi/code/csscomb.js/lib/csscomb.js:3:12)
    at Module._compile (module.js:413:34)
    at normalLoader (/Users/ravasthi/code/csscomb.js/node_modules/babel-core/lib/api/register/node.js:199:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/ravasthi/code/csscomb.js/node_modules/babel-core/lib/api/register/node.js:216:7)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)

I'm running using node v5.5.0 and npm v3.6.0. Any ideas?

haoqunjiang commented 8 years ago

@ravasthi Pushed a commit in the dev branch just now. Check it out and it should be ok now.

Currently the development workflow of csscomb.js is not so user friendly. Since the two main upstream dependencies (csscomb-core & gonzales-pe) are still in development and haven't been released to npm yet, you have to manually checkout these packages from github, run build scripts and then npm link them.

I've included these scripts in the .travis.yml file previously and now moved to package.json. So just take another try.

Edit: the newest building scripts are ok with npm@2, but not compatible with npm@3. But I don't have enough time to fix that at the moment since it's too late in China and I still have to go to work tomorrow. So please install npm@2 to work on this project for now, or wait till tomorrow and I'll get it fixed.

ravasthi commented 8 years ago

@sodatea thanks, much appreciated. I won't be able to work on it until tonight (I'm on the west coast of the US) anyway because of work, so I'll just wait.

haoqunjiang commented 8 years ago

@ravasthi Got it fixed. You can pull from the dev branch and run npm i now. (It may take some time to finish, since csscomb-core and gonzales-pe are both built locally.)

ravasthi commented 8 years ago

Awesome, thanks! I'll get a new PR in ASAP.

haoqunjiang commented 8 years ago

Closed in favor of https://github.com/csscomb/csscomb.js/pull/450