Skip to content

Conversation

@faceleg
Copy link

@faceleg faceleg commented May 19, 2015

Please review and let me know if any changes are required before merging

@faceleg faceleg mentioned this pull request May 19, 2015
@istro
Copy link

istro commented May 21, 2015

👍
Thank you!
For the time being I just cloned your repo, but this should definitely get addressed soon :-)

@istro
Copy link

istro commented May 21, 2015

Not sure how it used to work with old implementation - but on errors would it notify me?

An error (which I also get when csscomb is run in command line) is this:

Please check the validity of the block starting from line #25

23 |     margin-right: $margin-default;
25*|     // We need the extra specification to override bootstrap
26 |     > a {
27 |       padding: $margin-small $margin-tab;

Gonzales PE version: 3.0.0-28
Syntax: scss
CSScomb Core version: 3.0.0

Comment is actually fine I think, but in any case - CLI gives an error, shouldn't plugin also do something other than fail silently?

@faceleg
Copy link
Author

faceleg commented May 21, 2015

You're right - I only tried it on vanilla CSS that passed linting (I use syntastic to lint css, I guess that runs before this plugin).

I'll look at adding error output when I get a chance

@istro
Copy link

istro commented May 21, 2015

Yeah, it's fair to assume people would set up lint rules to match their csscomb rules, so I guess it's not that big of a deal. Probably not super high priority to add error notification here...

I actually just set up scss-lint (also uses syntastic) to lint my sass, it's AWESOME, hehe. Today's the cleanup day! now that all the styles are neat and combed lint will prevent me from messing it up 👍

@faceleg
Copy link
Author

faceleg commented May 21, 2015

One would hope that if someone is using csscomb they also know about linting, yes! But I still think it is a good idea to have the plugin at least output something when it barfs instead of doing it in silence.

@tonyganch
Copy link
Member

Ahoj! Sorry for delay, I'll take a look at this pr tomorrow.

README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Should not there be csscomb/vim-csscomb?

@faceleg
Copy link
Author

faceleg commented Jun 2, 2015

I've addressed the issues you noted @tonyganch!

@faceleg
Copy link
Author

faceleg commented Jun 2, 2015

@istro I've updated this PR to include error output in case of csscomb failure.

Note that it will output only the following portion of the csscomb output:

Please check the validity of the block starting from line #25

@istro
Copy link

istro commented Jun 2, 2015

👍

@faceleg
Copy link
Author

faceleg commented Jun 2, 2015

Note this PR should fix: #6, #7, #8

Copy link
Member

Choose a reason for hiding this comment

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

Great idea to show errors

Copy link
Author

Choose a reason for hiding this comment

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

Thanks to @istro

tonyganch added a commit that referenced this pull request Jun 4, 2015
Updated plugin to use the nodejs csscomb implementation
@tonyganch tonyganch merged commit 497786f into csscomb:master Jun 4, 2015
@tonyganch
Copy link
Member

Thank you so much for this pr!

@faceleg
Copy link
Author

faceleg commented Jun 4, 2015

No problem, thanks for merging! 👯

@faceleg faceleg deleted the feature/javascript branch June 4, 2015 21:50
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.

3 participants