-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix custom_dir
support and minification inconsistency
#27
Fix custom_dir
support and minification inconsistency
#27
Conversation
No feedback and no merge, and I've seen the other PR merged 🤔Am I supposed to change something here @wilhelmer? |
To be honest, I don't know what to do with this. What's the issue with the |
This PR is safe to merge, it introduces no breaking changes for the end-user. The path in my - minify:
minify_html: !ENV [GMC_ENABLE_ON_PUBLISH, False]
minify_css: !ENV [GMC_ENABLE_ON_PUBLISH, False]
minify_js: !ENV [GMC_ENABLE_ON_PUBLISH, False]
cache_safe: !ENV [GMC_ENABLE_ON_PUBLISH, False]
htmlmin_opts:
remove_comments: true
css_files:
- assets/stylesheets/extra.css
js_files:
- assets/javascripts/extra.js The difference is that I moved all of my shared assets into the mkdocs-minify-plugin/mkdocs_minify_plugin/plugin.py Lines 148 to 150 in cecdab8
because it expected that the file will be located within the docs_dir , but right now it's not the case, so I fixed it by finding the first valid file path in a custom_dir of the theme aka the overrides directory in my case.
The second issue was that the new mkdocs-minify-plugin/mkdocs_minify_plugin/plugin.py Lines 147 to 157 in e0ab4d9
and you probably want to handle the js_min the same way even if the cache_safe setting is disabled 😄.The error was hidden because the current tests didn't properly check JavaScript minification with different settings enabled. |
if self.config["cache_safe"]: | ||
docs_file_path: str = f"{config['docs_dir']}/{file_path}".replace("\\", "/") | ||
|
||
for user_config in config.user_configs: |
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 caught me off guard that someone is actually using user_configs
in MkDocs. I thought it's there in MkDocs only by accident. Could you explain why it was used here, I'm curious. I thought that one could just read config.theme
directly and get the same outcome.
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 used it since otherwise I would have to use config.theme._vars
, and since it's a pythonic-private variable I preferred to use a public one.
I did the same in the social cards plugin in MkDocs Material.
Hello again,
my team started supporting multiple languages in the docs, and I moved the shared extra assets to our overrides directory to avoid file duplication. This created an issue with the
cache_safe
option. Seems like people don't use this setting, because otherwise this issue would be found sooner. Maybe people prefer less "invasive" ways like this one, however I'm not sure if that's all there is to it.Additionally, I've unified the minification when using and not using the
cache_safe
option.The order of functions makes the code still hard to read and maybe has caused the bug in the first place
My attempt at restructuring wasn't accepted, because I did too many things back then #19 and then opted into a less invasive revision, but I still think it's worth a shot.
You could add some "easy-first-commit" issues to: