-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add code syntax theme to docs #405
Conversation
we've been highlighting this whole time, just not coloring. tried to minimize magic in code, but i couldn't help myself a few times.
); | ||
|
||
|
||
@mixin syntax($scopes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: calling this argument "scopes" is a little confusing; I would just call it "colors" or something
} | ||
|
||
&.pseudo-element { | ||
@extend .tag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be a lint error? we should be using the placeholder-in-extends
rule..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we disabled that nonsense cuz we moved our placeholders to mixins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why that justifies disabling the rule? If we aren't using placeholders, then there should really be no reason to use @extends
at all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll notice i didn't disable the rule here. it's always been like this.
this seems like a valid use of @extend
: i want one selector to behave exactly like another, in a very controlled way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searching for usages of @extend
reveals more instances of what i'm doing here: https://github.com/palantir/blueprint/search?utf8=%E2%9C%93&q=extend&type=Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know it hasn't been disabled inline, I'm just alarmed that this passed lint because I expected the rule to be enabled across the codebase.
There's a good reason the lint rule is there. Basically every time it's been circumvented has been regretful. But this is in the docs site so I don't mind being lenient about it. Still should be accompanied with a stylelint-disable
flag to make it stand out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, now we have an objectively better solution that involves a whopping zero @extend
s 🛠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found an unrelated bug (#412), but lgtm!
Fixes #98