-
Notifications
You must be signed in to change notification settings - Fork 953
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
base: next
Are you sure you want to change the base?
Conversation
* 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.
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 like where this is going!
|
||
std::fs::write(dir.path().join("zola.scss"), format!("$config: {}", config_serialized))?; | ||
|
||
Ok(dir) |
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.
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
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.
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.
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 it would be ok to check it in? It's not going to change very often
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.
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?
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.
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
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.
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?
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.
Honestly I'm not too sure. Maybe it's ok to just show an example of how it looks like in the docs
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)`.
@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> { |
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 don't understand the name of that function.
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.
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?
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.
write_temp_config_file
?
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.
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.
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.
ah fair enough create_config_load_path
?
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 like it
|
||
std::fs::write(dir.path().join("zola.scss"), format!("$config: {}", config_serialized))?; | ||
|
||
Ok(dir) |
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.
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; |
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.
what if the string contains "
?
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.
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?
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.
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...
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.
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; |
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.
You can use the type here so you don't repeat multiple times the same impl for different kinds like tuple/seq
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.
See https://github.com/Keats/tera2/blob/master/src/value/ser.rs#L16 for an example
} | ||
|
||
// Same thing but for tuple structs. | ||
impl<'a> serde::ser::SerializeTupleStruct for &'a mut SassMapSerializer { |
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.
See my comment above, you don't need to repeat it like that
@greenwoodcm do you think you will have time to address the comments? |
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 |
No worries, I'm on paternity leave myself so not super active either |
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
Hi, could anyone take a look at my solution? |
Looks good! Are you sure you need to use the grass-compiler crate though? |
Yes, as grass does not expose some types (Builtin, Value, etc.) required for add_custom_fn. ref |
Interesting. I would maybe raise an issue on the grass repo since this exact usecase might be somewhat common |
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:
Code changes
(Delete or ignore this section for documentation changes)
next
branch?If the change is a new feature or adding to/changing an existing one: