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

Change/Fix: Include "theme-css/*.scss" files #429

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

alphapapa
Copy link
Contributor

This includes Sass files in the theme-css directory (previously they were only included from the downstream css directory).

To be used in #418.

Copy link

netlify bot commented Nov 29, 2023

Deploy Preview for scientific-python-hugo-theme ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 72382e9
🔍 Latest deploy log https://app.netlify.com/sites/scientific-python-hugo-theme/deploys/65679562fea6e800087e4ce7
😎 Deploy Preview https://deploy-preview-429--scientific-python-hugo-theme.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 82
Accessibility: 100
Best Practices: 100
SEO: 92
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@alphapapa alphapapa self-assigned this Nov 29, 2023
@alphapapa alphapapa added this to the 0.5 milestone Nov 29, 2023
@alphapapa alphapapa added the type: Bug fix Something isn't working label Nov 29, 2023
@stefanv
Copy link
Member

stefanv commented Nov 29, 2023

Note the review comment on **/* syntax being supported.

See <scientific-python#429 (comment)>.

Suggested-by: Stefan van der Walt <stefanv@berkeley.edu>
@alphapapa
Copy link
Contributor Author

Note the review comment on **/* syntax being supported.

@stefanv Thanks, I applied that glob pattern to all the relevant places in this file.

Is this causing the "Undefined mixin" errors from TOCSS-DART?
@alphapapa
Copy link
Contributor Author

alphapapa commented Nov 29, 2023

@stefanv Using the **/* glob pattern caused Undefined mixin errors which broke the build. I'm not sure what's going on there, but after I reverted those changes in 72382e9, the build succeeded again.

#418 is waiting on this to be merged, so if you'd like me to look into the issues with that glob pattern, I could do that in a separate issue.

Comment on lines +48 to +54
{{ if $inServerMode -}}
{{ $custom_style := . | resources.ExecuteAsTemplate . $page -}}
<link rel="stylesheet" href="{{ $custom_style.RelPermalink }}">
{{ else }}
{{ $custom_style := . | resources.ExecuteAsTemplate . $page | minify | fingerprint -}}
<link rel="stylesheet" href="{{ $custom_style.RelPermalink }}" integrity="{{ $custom_style.Data.Integrity }}">
{{- end -}}
Copy link
Member

@stefanv stefanv Nov 29, 2023

Choose a reason for hiding this comment

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

The reason for this indentation is because of the generated HTML—with this change, the generated file with have spaces before the <link> tags.
I'm OK with keeping it as-is, but my emacs also doesn't indent that way by default.
Perhaps you have settings I can adopt?

Copy link
Contributor Author

@alphapapa alphapapa Nov 29, 2023

Choose a reason for hiding this comment

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

I indented these lines manually for clarity in the source code (the {{- end -}} repeated 3 lines in a row, with different opening lines for each block, is hard to follow). But just let me know how you want this to be done.

@stefanv
Copy link
Member

stefanv commented Nov 29, 2023

@stefanv Using the **/* glob pattern caused Undefined mixin errors which broke the build. I'm not sure what's going on there, but after I reverted those changes in 72382e9, the build succeeded again.

#418 is waiting on this to be merged, so if you'd like me to look into the issues with that glob pattern, I could do that in a separate issue.

It probably means that we should only load top-level PDST SCSS, which then itself imports deeper down.

@alphapapa
Copy link
Contributor Author

@stefanv Using the **/* glob pattern caused Undefined mixin errors which broke the build. I'm not sure what's going on there, but after I reverted those changes in 72382e9, the build succeeded again.
#418 is waiting on this to be merged, so if you'd like me to look into the issues with that glob pattern, I could do that in a separate issue.

It probably means that we should only load top-level PDST SCSS, which then itself imports deeper down.

Ok, shall we handle changing the glob patterns in a separate issue then? It also seems like a bit of a design or policy decision, whether to use or allow subdirectories for downstream stylesheet files.

@stefanv stefanv merged commit b98c9f6 into scientific-python:main Nov 29, 2023
4 checks passed
@stefanv
Copy link
Member

stefanv commented Nov 29, 2023

Thanks, let's get this in so the copy button PR can be rebased.

@alphapapa alphapapa deleted the wip/theme-css-sass branch December 1, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Bug fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants