Skip to content
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

Fix SCSS lint errors in craft/style.scss #15380

Merged
merged 1 commit into from May 25, 2017
Merged

Conversation

islemaster
Copy link
Contributor

Hound complained about some SCSS lint errors in craft/style.scss on this DTT which it's not supposed to do so we might disable Hound for SCSS altogether. However, these were easy to clean up (and not a bad idea) so I'm cleaning them up anyway.

@@ -400,7 +405,7 @@ $height-to-width: 434 / 477;

#craft-popup-connect {
@include reset-dialog;
background-color: #3C3C3C;
background-color: $almost-very-dark-gray;
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, our color names

Copy link
Contributor

Choose a reason for hiding this comment

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

With names like these, are we really making things that much clearer by using english vs. hex codes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about clearer, but I think it's useful in the sense that you end up with this block near the top of the file:

$darkest-gray: #333;
$very-dark-gray: #3b3b3b;
$almost-very-dark-gray: #3c3c3c;

It amplifies a couple of style-smells: That we are using three near-identical colors, and that we don't have a semantic name for any of them (where colors $button-text-white and $header-text-yellow are more obviously useful because it's clear when/where to use them in the future). I'm not bothering to fix that here because I'd like this to be a pure refactor, but I think it's more likely to get cleaned up next time somebody works on these colors if they're grouped like that.

@islemaster islemaster merged commit 123b567 into staging May 25, 2017
@islemaster islemaster deleted the fix-color-lint-errors branch May 25, 2017 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants