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

Add new _sass/custom/setup.scss for variable definition #1135

Merged
merged 4 commits into from Jan 18, 2023

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Jan 17, 2023

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 our support 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 for setup.scss, I've opted to not include it. If we think this is relevant, I can re-add it.

$pink-100: #f967f1;
$pink-200: #e94ee1;
$pink-300: #dd2cd4;
// custom CSS rules goes here
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// custom CSS rules goes here
// custom SCSS rules go here

Copy link
Member Author

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)

Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines 2 to 6

$pink-000: #f77ef1;
$pink-100: #f967f1;
$pink-200: #e94ee1;
$pink-300: #dd2cd4;
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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…

docs/customization.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pdmosses pdmosses left a 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>
@mattxwang
Copy link
Member Author

Thanks for the laser-quick review @pdmosses; think I've addressed each comment.

Copy link
Contributor

@pdmosses pdmosses left a 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.

@mattxwang mattxwang merged commit d423c96 into main Jan 18, 2023
@mattxwang mattxwang deleted the split-up-custom-scss branch January 18, 2023 19:13
@mattxwang mattxwang mentioned this pull request Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom callout colors no longer work
2 participants