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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix broken highlighting for properties sharing selector names #234

Merged
merged 5 commits into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@dsifford
Contributor

dsifford commented Oct 2, 2017

Hi there,

This PR addresses a small issue in the scss grammar that caused property names that match selector names to be highlighted incorrectly.

The fix simply involved switching the order of the includes.

The bug can be reproduced using the current grammar by typing this into a scss file...

.foo {
  color: red; // no issue
  font: 'Helvetica'; // issue
  cursor: pointer; // issue
  content: ''; // issue
}

Let me know if you need anything else. Thanks so much 馃槃

Edit:

Fixes #226

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

Thanks so much for the PR @dsifford! Can you take a look at the failing specs? Hopefully those can be fixed easily as well.

Please tell me if you need any help running specs.

Member

50Wliu commented Oct 2, 2017

Thanks so much for the PR @dsifford! Can you take a look at the failing specs? Hopefully those can be fixed easily as well.

Please tell me if you need any help running specs.

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 2, 2017

Contributor

Yep, looking at them now. 馃槃

Looks like it should be fairly straightforward to fix as well.

Is there any way to run these tests locally? I plan on hammering out a ton of random bugs in various languages throughout atom as time allows so any tips you might have on my first contribution are greatly encouraged and appreciated!

Contributor

dsifford commented Oct 2, 2017

Yep, looking at them now. 馃槃

Looks like it should be fairly straightforward to fix as well.

Is there any way to run these tests locally? I plan on hammering out a ton of random bugs in various languages throughout atom as time allows so any tips you might have on my first contribution are greatly encouraged and appreciated!

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

Is there any way to run these tests locally?

atom --test spec. To only run specific tests, add an f in front of them: fdescribe, fit. You can focus multiple tests.

I plan on hammering out a ton of random bugs in various languages throughout atom as time allows

鉂わ笍 馃槏 鉂わ笍 It'll be nice having some help!

Member

50Wliu commented Oct 2, 2017

Is there any way to run these tests locally?

atom --test spec. To only run specific tests, add an f in front of them: fdescribe, fit. You can focus multiple tests.

I plan on hammering out a ton of random bugs in various languages throughout atom as time allows

鉂わ笍 馃槏 鉂わ笍 It'll be nice having some help!

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 2, 2017

Contributor

Aha, so the test runner is built into atom itself. How fancy!

Thanks for the help. Busy for the rest of the night, but I'll try to circle back to this tomorrow or the next day 馃憤

Contributor

dsifford commented Oct 2, 2017

Aha, so the test runner is built into atom itself. How fancy!

Thanks for the help. Busy for the rest of the night, but I'll try to circle back to this tomorrow or the next day 馃憤

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 3, 2017

Contributor

@50Wliu I think I got it 馃憤

Edit: Just realized I neglected to add tests for the changes. I'll do that tomorrow afternoon.

Contributor

dsifford commented Oct 3, 2017

@50Wliu I think I got it 馃憤

Edit: Just realized I neglected to add tests for the changes. I'll do that tomorrow afternoon.

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 3, 2017

Contributor

Good to go

Contributor

dsifford commented Oct 3, 2017

Good to go

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 3, 2017

Member

Yay! Would you mind taking a look at language-less next, which has the same problem? I'll try to review this shortly.

Member

50Wliu commented Oct 3, 2017

Yay! Would you mind taking a look at language-less next, which has the same problem? I'll try to review this shortly.

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 3, 2017

Contributor

gif

I'm on it!

Contributor

dsifford commented Oct 3, 2017

gif

I'm on it!

@50Wliu 50Wliu merged commit 158c7fe into atom:master Oct 4, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment