Skip to content

Rename sass partial created for new blank site#9257

Merged
jekyllbot merged 6 commits intojekyll:masterfrom
ashmaroli:blank-site-sass-partial
Jan 19, 2023
Merged

Rename sass partial created for new blank site#9257
jekyllbot merged 6 commits intojekyll:masterfrom
ashmaroli:blank-site-sass-partial

Conversation

@ashmaroli
Copy link
Copy Markdown
Member

  • This is a 🐛 bug fix.
  • I've added tests.

Summary

jekyll-sass-converter-3.0.0 dropped support for importing sass partial named the same as the calling sass file.
Therefore, rename the partial _sass/main.scss to _sass/base.scss.

Context

Resolves #9250

@ashmaroli ashmaroli added fix backport-candidate Consider for merge into an older stable branch labels Jan 15, 2023
@ashmaroli ashmaroli requested a review from parkr January 15, 2023 16:47

print_label "Creating new BLANK site"
rm -Rf ./tmp/blank-site
bundle exec jekyll new tmp/blank-site --blank
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm hesitant to duplicate script/default-site like this... I'm wondering if we can either reuse script/default-site for 99% of this script, or, better yet, use a Cucumber test for this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Cucumber is my favorite option here an alternative is to re-use default-site: #9259.

@ashmaroli
Copy link
Copy Markdown
Member Author

I think Cucumber is my favorite option here

@parkr I have enhanced the Cucumber feature and removed duplicated shell script.

@ashmaroli ashmaroli requested a review from parkr January 18, 2023 14:55
And the test_blank/assets/css directory should exist
And the "test_blank/_layouts/default.html" file should exist
And the "test_blank/_sass/main.scss" file should exist
And the "test_blank/_sass/base.scss" file should exist
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So to be clear, this test was failing with main.scss but doesn't with base.scss? If so, then let's ship this PR!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that's correct.
dart-sass and therefore jekyll-sass-converter-3.0 doesn't allow css/foo.scss to import _sass/foo.scss.

@ashmaroli
Copy link
Copy Markdown
Member Author

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 3a18480 into jekyll:master Jan 19, 2023
@jekyllbot jekyllbot added the bug label Jan 19, 2023
jekyllbot added a commit that referenced this pull request Jan 19, 2023
@ashmaroli ashmaroli deleted the blank-site-sass-partial branch January 19, 2023 07:00
github-actions bot pushed a commit that referenced this pull request Jan 19, 2023
Ashwin Maroli: Rename sass partial created for new blank site (#9257)

Merge pull request 9257
ashmaroli added a commit to ashmaroli/jekyll that referenced this pull request Jan 19, 2023
ashmaroli added a commit that referenced this pull request Jan 19, 2023
@jekyll jekyll locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport-candidate Consider for merge into an older stable branch bug fix frozen-due-to-age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: jekyll new --blank creates non-working scaffold due to sass naming

3 participants