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

Improve mixin scopes #82

Merged
merged 12 commits into from Oct 25, 2017

Conversation

Projects
None yet
2 participants
@dsifford
Contributor

dsifford commented Oct 4, 2017

Work In Progress

Description of the Change

See #81

Todo

  • add .bracket.round to the punctuation classes (punctuation.definition.parameters.end.bracket.round.less)
  • revert back to key-value from operator.assignment for colon separating parameter name and default value
  • fix missing .meta.mixin.less scopes in numeric default parameters.
  • lots and lots of tests

Resolves #81

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 4, 2017

Member

FYI I will be gone starting tomorrow until the 16th. So please feel free to create PRs for the other issues you wanted to tackle and I will try to review them after I get back.

Member

50Wliu commented Oct 4, 2017

FYI I will be gone starting tomorrow until the 16th. So please feel free to create PRs for the other issues you wanted to tackle and I will try to review them after I get back.

'patterns': [
{
'match': ':'
'name': 'punctuation.separator.key-value.less'

This comment has been minimized.

@dsifford

dsifford Oct 4, 2017

Contributor

@50Wliu Should this be punctuation.separator.key-value.css? Or is it incorrectly set here: https://github.com/atom/language-less/pull/82/files#diff-14c55924b6cf1fe75bf40519b9cddaccR317?

@dsifford

dsifford Oct 4, 2017

Contributor

@50Wliu Should this be punctuation.separator.key-value.css? Or is it incorrectly set here: https://github.com/atom/language-less/pull/82/files#diff-14c55924b6cf1fe75bf40519b9cddaccR317?

This comment has been minimized.

@50Wliu

50Wliu Oct 16, 2017

Member

I think this can be kept as .less.

@50Wliu

50Wliu Oct 16, 2017

Member

I think this can be kept as .less.

This comment has been minimized.

@dsifford

dsifford Oct 16, 2017

Contributor

Should I change the other occurrence then?

@dsifford

dsifford Oct 16, 2017

Contributor

Should I change the other occurrence then?

This comment has been minimized.

@50Wliu

50Wliu Oct 16, 2017

Member

The other one can be kept as .css because that one is the same in CSS, whereas for this one there is no function equivalent in CSS.

It's a bit confusing and is pretty ad-hoc. Hopefully in the future the endings can be standardized.

@50Wliu

50Wliu Oct 16, 2017

Member

The other one can be kept as .css because that one is the same in CSS, whereas for this one there is no function equivalent in CSS.

It's a bit confusing and is pretty ad-hoc. Hopefully in the future the endings can be standardized.

dsifford added some commits Oct 5, 2017

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 5, 2017

Contributor

@50Wliu Got all the current tests passing. I'll hold off on going any further for now until I can get confirmation from you that the structure looks acceptable.

Enjoy your vacation!

Contributor

dsifford commented Oct 5, 2017

@50Wliu Got all the current tests passing. I'll hold off on going any further for now until I can get confirmation from you that the structure looks acceptable.

Enjoy your vacation!

Show outdated Hide outdated grammars/less.cson Outdated
Show outdated Hide outdated grammars/less.cson Outdated
Show outdated Hide outdated grammars/less.cson Outdated
Show outdated Hide outdated grammars/less.cson Outdated
Show outdated Hide outdated grammars/less.cson Outdated
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 16, 2017

Member

Can you please revert the spec structure to how it was originally?

Member

50Wliu commented Oct 16, 2017

Can you please revert the spec structure to how it was originally?

@50Wliu 50Wliu added the under-review label Oct 16, 2017

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 16, 2017

Contributor

@50Wliu Welcome back!

Thanks for looking this over. I'll address these issues soon.

Re: crappy looking tests -- that was just temporary until you had time to get your eyes on it because I was being lazy 😝. I'll definitely fix that!

Contributor

dsifford commented Oct 16, 2017

@50Wliu Welcome back!

Thanks for looking this over. I'll address these issues soon.

Re: crappy looking tests -- that was just temporary until you had time to get your eyes on it because I was being lazy 😝. I'll definitely fix that!

dsifford added some commits Oct 17, 2017

fix all issues mentioned in PR
- still need to fix and add tests
@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Oct 19, 2017

Contributor

@50Wliu Should be ready for review now. Let me know if I missed something.

Contributor

dsifford commented Oct 19, 2017

@50Wliu Should be ready for review now. Let me know if I missed something.

Show outdated Hide outdated spec/less-spec.coffee Outdated
Show outdated Hide outdated grammars/less.cson Outdated
Show outdated Hide outdated grammars/less.cson Outdated

@50Wliu 50Wliu merged commit 2802ab2 into atom:master Oct 25, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 28, 2017

Member

@dsifford Sorry to bother you again, but here's another regression I'm seeing:

.markdown-preview[data-use-github-style] {
  .markdown-body(); // comment this out to fix everything

  padding: 30px; // padding is property-value
  font-size: 16px; // entity.name.tag.custom
  color: #333; // support.type.property-name.media
  background-color: #fff; // entity.name.tag.custom
  overflow: scroll; // no scope

  a {
    color: #337ab7;
  }
}
Member

50Wliu commented Oct 28, 2017

@dsifford Sorry to bother you again, but here's another regression I'm seeing:

.markdown-preview[data-use-github-style] {
  .markdown-body(); // comment this out to fix everything

  padding: 30px; // padding is property-value
  font-size: 16px; // entity.name.tag.custom
  color: #333; // support.type.property-name.media
  background-color: #fff; // entity.name.tag.custom
  overflow: scroll; // no scope

  a {
    color: #337ab7;
  }
}

@50Wliu 50Wliu referenced this pull request Nov 1, 2017

Merged

Revert "Improve mixin scopes" #84

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 1, 2017

Member

Reverted for the time being.

Member

50Wliu commented Nov 1, 2017

Reverted for the time being.

@dsifford

This comment has been minimized.

Show comment
Hide comment
@dsifford

dsifford Nov 1, 2017

Contributor

Sorry... this got buried in my notifications. I'll try to look at this this weekend

Contributor

dsifford commented Nov 1, 2017

Sorry... this got buried in my notifications. I'll try to look at this this weekend

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 1, 2017

Member

Yeah no problem! I'm hoping that I'll have time to investigate as well.

Member

50Wliu commented Nov 1, 2017

Yeah no problem! I'm hoping that I'll have time to investigate as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment