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

fix(assets): Handle more assets #256

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

kamilkrzyskow
Copy link
Contributor

@kamilkrzyskow kamilkrzyskow commented Sep 7, 2023

Some assets (like images) might stay the same across different language builds, as in no localized version is available.
Currently only the default language adds the files to the internal files list and built alternates show a warning that a file isn't found in the documentation files.

This PR tries to fix just that by always including any non .md file to the build, so that would include .png .jpg .js etc. files.
As far as I know, the files aren't being duplicated in the alternate builds, just that the path to them is properly resolved.

It partially helps with #251

EDIT:
Actually, unrelated to #252

@kamilkrzyskow kamilkrzyskow mentioned this pull request Sep 7, 2023
@ultrabug
Copy link
Owner

ultrabug commented Sep 8, 2023

The issue with #252 is unfortunately not related to this PR as per my comment

Can you elaborate on what if fixes exactly on #251 please? A simple and clear example would help me a lot!

@kamilkrzyskow
Copy link
Contributor Author

Bummer 😞, it definitely fixes my issue where the assets are placed in the overrides, because I tested it with my setup.

Issue 1 from #251 as explained in the first comment.

Today I won't be at my PC so I won't be be able to create any example till tomorrow.

@AngryMane
Copy link
Contributor

AngryMane commented Sep 9, 2023

@kamilkrzyskow
Hmmm. If you want to implement this plan, I think you need to modify the dest_path and abs_dest_path of the file as follows.

elif self.is_default_language_build or not file.src_uri.endswith(".md"):
                import os
                locale_site_dir = self.current_language if self.current_language ! = self.default_language else ""
                file.dest_path = Path(locale_site_dir) / file.dest_path
                file.abs_dest_path = os.path.normpath(os.path.join(mkdocs_config.site_dir, file.dest_path))

However, this plan would not accommodate cases where various files in various directories load the same template.

$ tree
.
├── docs
│   ├── index.cs.md
│   ├── index.md
│   ├── index.pl.md
│   └── nested
│       ├── nested.cs.md
│       ├── nested.md
│       └── nested.pl.md
├── mkdocs.yml
└── overrides
    ├── assets
    │   └── images
    │       └── test.drawio.svg
    └── main.html

5 directories, 9 files

At this time, if all files index.md, index.pl.md, index.cs.md, nested/nested.md, nested/nested.pl.md, nested/nested.cs.md refer to the main.html template, the main. I don't think I can represent the path from html to test.drawio.svg statically.

sample project is here -> https://github.com/AngryMane/sample-mkdocs-prj

@AngryMane
Copy link
Contributor

AngryMane commented Sep 9, 2023

@kamilkrzyskow
I'm not sure, but your raising of the issue may point to a leak in mkdocs' spec consideration.
it seems to me that the paths to the links listed in the template should be corrected for each file that renders.
In fact, in md files, the link paths are modified.
https://github.com/mkdocs/mkdocs/blob/3db7b8c9c2473e763fc77525ee17c42e965d3b91/mkdocs/structure/pages.py#L425

@AngryMane
Copy link
Contributor

AngryMane commented Sep 9, 2023

@ultrabug's suggestion may be a smart way to handle this.

or, use abs path like '<img src="/assets/images/test.drawio.svg" width="100" height="100" />'

@kamilkrzyskow
Copy link
Contributor Author

OK, AngryMane's example and Ultrabug's solution made it clear that this PR indeed doesn't fix #252 as the hardcoded links, in the html templates, are unaffected by the changes.

I agree, that Ultrabug's suggestion seems to be the most optimal, as for the abs path like /assets/images/test.drawio.svg in the HTML, it won't work with most GitHub Pages, because the link has a structure like username.github.io/repository/. So using the abs method requires to keep the real base path in mind.
Perhaps using the site_url MkDocs setting to inject it in Jinja2 might also work {{ config.site_url }}/assets/images/test.drawio.svg.


Moving back on topic of this PR:
@AngryMane, thanks a lot for your time and for creating an example project in my stead. My first issue from #251 is unrelated to templates, so I've adjusted the example:
i18n-regression.zip

No matter if it's nested or not, when building alternates MkDocs logs a warning:

WARNING -  Doc file 'index.pl.md' contains a relative link 'assets/images/test.drawio.svg', but the target is not found among documentation files.
WARNING -  Doc file 'nested/nested.pl.md' contains a relative link '../assets/images/test.drawio.svg', but the target 'assets/images/test.drawio.svg' is not found among documentation files.

The warning stems from the fact that alternates don't add the assets directory to the i18n_files list and MkDocs uses that list to search for paths referenced in Markdown.

I hope this cleared things up @ultrabug

Copy link
Owner

@ultrabug ultrabug left a comment

Choose a reason for hiding this comment

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

Thanks @AngryMane and @kamilkrzyskow

I understand better now what you're trying to fix and thank you for providing everything.

I'd just like to propose this version instead which looks clearer to me?

mkdocs_static_i18n/reconfigure.py Outdated Show resolved Hide resolved
@ultrabug ultrabug merged commit b79eaf4 into ultrabug:main Sep 14, 2023
12 checks passed
@ultrabug
Copy link
Owner

Thanks @kamilkrzyskow and @AngryMane

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.

3 participants