-
-
Notifications
You must be signed in to change notification settings - Fork 30
fix(material_social): use cards_dir to build cards url for page #354
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
+ Coverage 81.12% 81.46% +0.34%
==========================================
Files 11 11
Lines 747 750 +3
Branches 125 124 -1
==========================================
+ Hits 606 611 +5
+ Misses 97 96 -1
+ Partials 44 43 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Hi @kanru , Thanks for your PR! Sorry, CI was broken so I've fixed it in #357. Can you rebase your branch on origin/main please? Can you also apply contributing guidelines, to make pre-commit.ci happy? I'll review it once everything is green here :). |
571701e to
0370428
Compare
|
|
@Guts done! |
|
Hey @kanru, I'm back to this project for a piece of free time and reviewing your PR, I found out that there is still no unit tests. So I've pushed it kanru-contrib#1 |
Guts
left a comment
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.
LGTM bu still waiting for unit tests. See kanru-contrib#1
|
Thanks for spending time on maintaining this project! I don't really write any python so just figuring out how to fix the issue and use the right style had already exhausted me. Thanks for writing the unit tests. |
|
Thanks @kanru. The failing test is unrelated so I'll fix it later in a future PR. |
| # if page is a blog post | ||
| if ( | ||
| self.integration_material_blog.IS_BLOG_PLUGIN_ENABLED | ||
| and self.integration_material_blog.is_page_a_blog_post(mkdocs_page) | ||
| ): | ||
| page_social_card = ( | ||
| f"{mkdocs_site_url}assets/images/social/" | ||
| f"{Path(mkdocs_page.file.dest_uri).parent}.png" | ||
| ) | ||
| else: | ||
| page_social_card = ( | ||
| f"{mkdocs_site_url}assets/images/social/" | ||
| f"{Path(mkdocs_page.file.src_uri).with_suffix('.png')}" | ||
| ) |
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.
Finally this introduced a bug with blog posts social cards.
| # As of mkdocs-material 9.6.5, social cards are always stored in the | ||
| # matching src path in the build folder, regardless of the page type. |
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.
This assertion is wrong. To reproduce:
mkdocs build -f ./tests/fixtures/mkdocs_item_image_social_cards_blog.yml --verbose


This should fix #319
The path building is based on https://github.com/squidfunk/mkdocs-material/blob/396c493f4774fa0bafeada5d0b555d02193557e8/src/plugins/social/plugin.py#L339-L362