Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Separate markup styles from gfm #50

Merged
merged 4 commits into from
Oct 16, 2015
Merged

Separate markup styles from gfm #50

merged 4 commits into from
Oct 16, 2015

Conversation

burodepeper
Copy link
Contributor

While making language-markdown as compatible as possible with current syntax themes, I noticed that some styling defined specific for source.gfm might be better off as part of the general .markup class.

I believe this change doesn't break anything, but might perhaps add some extra styling to documents that use the generic markup class. If you agree on this change, I think this should be changed in all default syntax themes. I used one-dark-syntax merely as an example.

One thing that I'm not 100% sure of, is the disabling of the .markup .list style. In my implementation of Markdown, .list is used to describe an entire list-item, instead of just the .list .variable that is actually meant to be targeted in the original.

@simurai
Copy link
Contributor

simurai commented Oct 8, 2015

I think it's a good idea to make it more general and not only for .gfm.

https://github.com/atom/apm/blob/master/templates/theme/styles/base.less is also a good place to change it. It's the styles that get used when generating a new syntax theme in Atom.

@burodepeper
Copy link
Contributor Author

Agreed.

Could you update the default syntax themes? (I think they all 'suffer' from this 'issue')
Then I'll take a jab at the template in apm in the morning.

}
// &.list {
// color: @hue-5;
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 Remove commented out lines

&.link {
color: @hue-3;
}

&.heading .punctuation.definition.heading {
color: @hue-2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nest this block inside the &.heading { .. } block above? Thus resulting in

&.heading {
  color: @hue-5;

  .punctuation.definition.heading {
    color: @hue-2;
  }
}

@thomasjo
Copy link
Contributor

thomasjo commented Oct 8, 2015

Now that my line comments have been resolved, this looks good to me! /cc @atom/feedback

@burodepeper
Copy link
Contributor Author

NOTE: changes need to be made to other default templates as well, but I'd like to leave that up to somebody who already has all eight (?) of them forked...

color: @hue-3;
.list {
.variable {
color: @hue-5;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems not to be needed, since those "list variables" already get styled as stand alone .variables.

I think the whole

.list {
  .variable {
    color: @hue-5;
  }
}

can be removed.

@burodepeper
Copy link
Contributor Author

@simurai was right, and I removed the obsolete selector.

@simurai
Copy link
Contributor

simurai commented Oct 16, 2015

Looks.. good. I'll try to update all the other core themes as well.

simurai added a commit that referenced this pull request Oct 16, 2015
Separate markup styles from gfm
@simurai simurai merged commit f481d06 into atom:master Oct 16, 2015
@burodepeper
Copy link
Contributor Author

👍

@burodepeper burodepeper deleted the markup branch October 16, 2015 07:09
simurai added a commit to atom/one-light-syntax that referenced this pull request Oct 16, 2015
simurai added a commit to atom/base16-tomorrow-dark-theme that referenced this pull request Oct 16, 2015
simurai added a commit to atom/base16-tomorrow-light-theme that referenced this pull request Oct 16, 2015
simurai added a commit to atom/solarized-light-syntax that referenced this pull request Oct 16, 2015
simurai added a commit to atom/solarized-dark-syntax that referenced this pull request Oct 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants