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(feed): fix "Visit website" link with skin config #286

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

be-next
Copy link
Contributor

@be-next be-next commented Mar 21, 2024

Summary

We've observed an inconsistent behavior in the generation of Atom feed links, specifically regarding the construction of the href attribute for the visit_the_site link. The issue arises depending on the skin variable's value in the config.toml file. When the skin variable is not set (i.e., left as an empty string ""), the XPath expression /atom:feed/atom:link[2]/@href successfully retrieves the intended URL stored in config.base_url. However, when the skin variable is initialized with a value (e.g., "red"), the mentioned XPath expression fails to function as intended, leading to incorrect href values in the generated Atom feed.

This behavior suggests that the insertion of an additional <link> element related to the skin configuration alters the document structure, affecting the index-based XPath selection.

Changes

To address this variability and ensure a more stable and reliable generation of the visit_the_site link, irrespective of the skin configuration or any other dynamic content that might affect the <link> elements order, we propose to directly use the /atom:feed/@xml:base XPath expression. This expression directly targets the xml:base attribute of the <feed> element, which consistently contains the config.base_url value.

This solution offers a more robust approach to determining the href attribute for the visit_the_site link, as it does not rely on the specific order or presence of <link> elements within the Atom feed. It ensures that the link always correctly points to the base URL as defined in the site's configuration, regardless of any changes to other parts of the feed structure.

The suggested change involves modifying the XSL transformation used to generate the Atom feed's HTML representation. Specifically, we will replace the /atom:feed/atom:link[2]/@href expression with /atom:feed/@xml:base for setting the href attribute of the visit_the_site link.

Type of change

  • Bug fix (fixes an issue without altering functionality)
  • New feature (adds non-breaking functionality)
  • Breaking change (alters existing functionality)
  • UI/UX improvement (enhances user interface without altering functionality)
  • Refactor (improves code quality without altering functionality)
  • Documentation update
  • Other (please describe below)

Checklist

  • I have verified the accessibility of my changes
  • I have tested all possible scenarios for this change
  • I have updated theme.toml with a sane default for the feature
  • I have made corresponding changes to the documentation:
    • Updated config.toml comments
    • Updated theme.toml comments
    • Updated "Mastering tabi" post in English
    • (Optional) Updated "Mastering tabi" post in Spanish
    • (Optional) Updated "Mastering tabi" post in Catalan

@be-next be-next requested a review from welpo as a code owner March 21, 2024 20:44
@be-next be-next changed the title Fix for inconsistent Atom Feed href behavior linked to skin configuration 🐛 fix: inconsistent Atom Feed href behavior Mar 21, 2024
@welpo
Copy link
Owner

welpo commented Mar 21, 2024

Nice catch! Thank you.

@welpo welpo changed the title 🐛 fix: inconsistent Atom Feed href behavior 🐛 fix(feed): correct Atom feed link with skin config Mar 21, 2024
@welpo welpo changed the title 🐛 fix(feed): correct Atom feed link with skin config 🐛 fix(feed): fix "Visit website" link with skin config Mar 21, 2024
@welpo welpo merged commit a08a9f1 into welpo:main Mar 21, 2024
5 of 6 checks passed
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.

2 participants