-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 new _sass/custom/setup.scss
for variable definition
#1135
Conversation
8934a60
to
673cb95
Compare
_sass/custom/custom.scss
Outdated
$pink-100: #f967f1; | ||
$pink-200: #e94ee1; | ||
$pink-300: #dd2cd4; | ||
// custom CSS rules goes here |
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.
// custom CSS rules goes here | |
// custom SCSS rules go here |
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'll provide an alternate more specific text; I did mean CSS rules (as in the formal spec definition)
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.
But the file extension is scss
, so I think Jekyll will compile it anyway?
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.
Oh, the idea wasn't to say that only CSS is allowed; rather, that everything in that file should emit CSS (after compilation), rather than just being variable declarations.
I think I'll revert this change somewhat since that's not actually a constraint.
_sass/custom/setup.scss
Outdated
|
||
$pink-000: #f77ef1; | ||
$pink-100: #f967f1; | ||
$pink-200: #e94ee1; | ||
$pink-300: #dd2cd4; |
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 should avoid content in such a custom file, because changing the content could break sites that rely on the content but don't override the file.
So I suggest to delete these lines.
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.
Hm, ok - in that case, this theme will introduce different behaviour (since we're removing these same lines from custom.scss
.
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 those lines were added in RC1, by #578. So it's not breaking wrt v0.3.3.
And yes, this illustrates very well the danger of putting content in custom styles…
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.
Apart from suggesting changes, I've updated https://just-the-docs.github.io/just-the-docs-tests/customization/custom-callout-color/ and checked that it passes when using this PR branch.
Co-authored-by: Peter Mosses <18308236+pdmosses@users.noreply.github.com>
Thanks for the laser-quick review @pdmosses; think I've addressed each comment. |
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.
LGTM. I'll recheck the test after it's been merged, then remove the ❌s and add a link to the test.
This is an alternative PR that resolves #1011. Unlike #1013, this PR defines a new SASS file,
_sass/custom/setup.scss
, specifically designed for new custom variables (and other SASS-only constructs). It's imported after oursupport
SASS files are (functions, variables), but otherwise is imported before all other files (ex, when CSS is emitted).So, custom callout colors can now be defined in this file. I also move the custom callout colors present in
custom.scss
to the right location.I've added some docs that briefly explain how to use the feature. Feedback is welcome!
cc: @Ethan0429
As an aside, I chose not to add a
_includes/css
file that imports this, and then import that file. I think that's only necessary if we're trying to render liquid somehow in the SASS file; since we're not trying to do that forsetup.scss
, I've opted to not include it. If we think this is relevant, I can re-add it.