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

inject site config during Sass compilation #2242

Open
wants to merge 12 commits into
base: next
Choose a base branch
from

Conversation

greenwoodcm
Copy link

IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?

Keats and others added 10 commits March 19, 2023 20:37
* Reuse Client when checking urls and add timeout for requests
* Implement replace_re filter

* Cargo fmt

* add regex caching

* cargo fmt

* update docs, update unit test

* rename replace_re -> regex_replace
Relative links in the entry content do not currently have a base URI, so
will be resolved relative to the feed URI:

Given an entry with the content:

    <a href="some-resource.bin">

And URIS of:

 * entry: https://example.org/blog/some-entry/
 * feed:  https://example.org/atom.xml

The link URI will end up as:

    https://example.org/some-resource.bin

rather than the URI that ends up resolved in the rendered page:

   https://example.org/blog/some-entry/some-resource.bin

The atom and RSS formats allow for an xml:base attribute (itself
specified in [1]) to provide a base URI of a subset of a document. This
change adds xml:base attributes to each entry, using the page permalink.

This gives us something equivalent to:

    <entry>
     <content xml:base="https://example.org/blog/some-entry/">
      <![CDATA[
       <a href="some-resource.bin">
      ]]>
     </content>
    </entry>

[1]: https://www.w3.org/TR/xmlbase/

Signed-off-by: Jeremy Kerr <[email protected]>
* Fix multi-ligual json index

* multi-lingual search Fix cargo fmt
…tzola#2196)

* Add search into the serialized config (getzola#2165)

* Only expose index_format

* Create config.search struct

* cargo fmt
* Fix hard link panic and add better error info to std:fs errors

* cargo fmt

* Remove erroneously committed config change

* Remove console import; Use with context to provide additional error info

* improve error wording
* Add optional decoding="async" loading="lazy" for img

In theory, they can make the page load faster and show content faster.

There’s one problem: CommonMark allows arbitrary inline elements in alt text.
If I want to get the correct alt text, I need to match every inline event.

I think most people will only use plain text, so I only match Event::Text.

* Add very basic test for img

This is the reason why we should use plain text when lazy_async_image is enabled.

* Explain lazy_async_image in documentation

* Add test with empty alt and special characters

I totaly forgot one can leave the alt text empty.
I thought I need to eliminate the alt attribute in that case,
but actually empty alt text is better than not having an alt attribute at all:
https://www.w3.org/TR/WCAG20-TECHS/H67.html
https://www.boia.org/blog/images-that-dont-need-alternative-text-still-need-alt-attributes
Thus I will leave the empty alt text.

Another test is added to ensure alt text is properly escaped.
I will remove the redundant escaping code after this commit.

* Remove manually escaping alt text

After removing the if-else inside the arm of Event::Text(text),
the alt text is still escaped.
Indeed they are redundant.

* Use insta for snapshot testing

`cargo insta review` looks cool!

I wanted to dedup the cases variable,
but my Rust skill is not good enough to declare a global vector.
this will improve module structure, in preparation for adding some
serde code in another sub-module.
Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

I like where this is going!


std::fs::write(dir.path().join("zola.scss"), format!("$config: {}", config_serialized))?;

Ok(dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of writing it to the sass directory so people can easily see what there is? It would be still overwritten on each serve/build but we can maybe not delete it

Copy link
Author

Choose a reason for hiding this comment

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

you're saying write it to the user's sass source directory? i figured that would be undesirable because then maybe it gets erroneously checked in. but i'm open to it if that seems better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be ok to check it in? It's not going to change very often

Copy link
Author

Choose a reason for hiding this comment

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

the more i think about it, the more i prefer not having the injected config in source. i don't particularly like it being checked in when the author didn't write it and when it's liable to get overwritten by the Zola compiler. that creates a scenario where the author is looking through the sass directory and erroneously edits that file, only to have their edits confusingly wiped out on the next zola build. we could solve that with a warning comment at the top of that file, but i prefer just hiding the source since the author isn't writing it. i added site docs explaining this feature, in my opinion that makes clear to the author what's going on. what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to not add it the directory but it's a bit annoying that you can't see it to look at what exactly is defined

Copy link
Author

Choose a reason for hiding this comment

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

any suggestions on how to surface this content to the site builder without putting it in the directory? i could create a non-tmp directory in /tmp and log the location of it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I'm not too sure. Maybe it's ok to just show an example of how it looks like in the docs

components/site/src/sass/serde.rs Show resolved Hide resolved
test_site/sass/blog.scss Outdated Show resolved Hide resolved
it is helpful to be able to use site config during Sass compilation,
for example to allow a site owner to set header color in a Sass
template.  in this change we serialize a subset of the site's config
to a `.scss` file as a Sass map literal, allowing config content to
be referenced in site Sass or theme Sass.

this commit involves bumping the `grass` dependency to version
`1.13.0`.  we do this so that we can use nested map retrieval in
Sass files, such as `map.get($config, extra, background_color)`.
@greenwoodcm
Copy link
Author

@Keats thoughts on this latest rev?

/// a load directory above when compiling the site's Sass files. the tempdir
/// and contained `.scss` file will be deleted on drop of the returned `TempDir`
/// struct, which should happen after Sass compilation finishes.
fn build_dependencies_dir_from_config(config: &Config) -> Result<TempDir> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the name of that function.

Copy link
Author

Choose a reason for hiding this comment

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

here we take the config document and construct a directory that contains files that are ultimately added as dependencies when compiling Sass. the Sass compiler asks for a "load dir" during compilation, so instead of providing a single file we must provide a single directory containing a single file. so the function name is basically "builds a dependencies directory from the config document". happy to rename, do you have a suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

write_temp_config_file?

Copy link
Author

Choose a reason for hiding this comment

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

that works for me. seems odd to me that a function would be named that but return a directory instead of a file, but whatever.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah fair enough create_config_load_path?

Copy link
Author

Choose a reason for hiding this comment

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

i like it


std::fs::write(dir.path().join("zola.scss"), format!("$config: {}", config_serialized))?;

Ok(dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to not add it the directory but it's a bit annoying that you can't see it to look at what exactly is defined


fn serialize_str(self, v: &str) -> std::result::Result<Self::Ok, Self::Error> {
self.output += "\"";
self.output += v;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if the string contains "?

Copy link
Author

Choose a reason for hiding this comment

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

great point. it seems like the most straightforward way to make this more robust is to first convert the String to a serde_json::Value::String and then write that out. Value::String will do the right thing regarding escapes, which should function the same for a Sass literal as for JSON.

the downside here is obviously taking a dependency on the serde_json crate. the alternative would be to simply check for the presence of characters that would require an escape and fail with a helpful error message. that also seems acceptable, as I can't think of a compelling reason why someone would need quotes or other escape characters in this context.

@Keats do you have a recommendation on which route to go?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do a naive escape (eg just a .replace)? It will do something weird if the user has an escaped string but that seems even less likely...

Copy link
Author

Choose a reason for hiding this comment

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

yeah i like that. i think what i'll do is naively escape double quotes and search for the characters that i know would need an escape, failing if i find any. that seems like a good happy medium between allowing the might-happen thing and disallowing the shouldn't-happen thing with a helpful message.

type Error = SassMapSerializerError;

type SerializeSeq = Self;
type SerializeTuple = Self;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the type here so you don't repeat multiple times the same impl for different kinds like tuple/seq

Copy link
Collaborator

Choose a reason for hiding this comment

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

}

// Same thing but for tuple structs.
impl<'a> serde::ser::SerializeTupleStruct for &'a mut SassMapSerializer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment above, you don't need to repeat it like that

@Keats
Copy link
Collaborator

Keats commented Nov 20, 2023

@greenwoodcm do you think you will have time to address the comments?

@greenwoodcm
Copy link
Author

oof, sorry this has been sitting forever, family life has me busy these days...i do plan on making these changes, thanks for the reminder

@Keats
Copy link
Collaborator

Keats commented Nov 22, 2023

No worries, I'm on paternity leave myself so not super active either

vimpostor added a commit to vimpostor/zola-aurora that referenced this pull request Jan 8, 2024
Some browsers (e.g. Chromium on Android) make use of the theme color to
highlight the tab bar in a different color.

For now we hardcode the same accent color as "--primary-color" in the
CSS files. In the future we should allow the consumer to configure the
accent color in the configuration file and get the value from there.

Unfortunately this is not possible at the moment, because zola is
missing support for CSS templating, but an implementation is already in
the pipeline [0], so with some luck we might soon be able to unify the
accent color retrieval.

[0] getzola/zola#2242
@e792a8
Copy link

e792a8 commented Jun 22, 2024

Hi, could anyone take a look at my solution?
It utilizes grass_compiler::Options::add_custom_fn() and then we just call in Sass $cfg : zola-config();. No need for any extra files.
If anyone is interested, I will open a new PR for that.

@Keats
Copy link
Collaborator

Keats commented Jun 23, 2024

Looks good! Are you sure you need to use the grass-compiler crate though?

@e792a8
Copy link

e792a8 commented Jun 24, 2024

Yes, as grass does not expose some types (Builtin, Value, etc.) required for add_custom_fn. ref

@Keats
Copy link
Collaborator

Keats commented Jun 24, 2024

Interesting. I would maybe raise an issue on the grass repo since this exact usecase might be somewhat common

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.