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

Simplify/correct definition links #3930

Merged
merged 4 commits into from
Jul 25, 2024
Merged

Simplify/correct definition links #3930

merged 4 commits into from
Jul 25, 2024

Conversation

patrickhlauke
Copy link
Member

Closes #3920

@patrickhlauke patrickhlauke marked this pull request as ready for review June 26, 2024 21:00
@kfranqueiro
Copy link
Contributor

This looks good to me for techniques and understanding. I would suggest removing the changes in this PR for conformance-challenges and guidelines since those aren't processed the same way. Sorry I was probably ambiguous about that in the issue.

@patrickhlauke
Copy link
Member Author

Reverted those two non-technique/non-understanding changes

@kfranqueiro
Copy link
Contributor

kfranqueiro commented Jul 1, 2024

TL;DR: we should not merge this until we're using the Eleventy build process, where I've fixed an underlying issue.

This isn't so much a problem with this PR as it is with the XSLT build process (the same issues already exist elsewhere in content). In the process of thoroughly testing, I think I understand why some of these (particularly some under techniques) were using full URLs.

The XSLT build process generates its own IDs for definitions links, disregarding the already-existing id attributes in the markup in guidelines/terms/* files. This is fine for Understanding pages, which cross-link to headings under its own Key Terms section. However, Techniques pages don't do this; they link directly to the TR - which is bad if the IDs don't match.

As it stands, the links to "status messages", "change of context", "conforming alternate version", and "web pages" will not scroll to the correct position in the TR when this PR's changes are processed by the XSLT build process. I have added a commit to the Eleventy build process PR (#3917) to address this for Techniques pages, so this PR will work 100% with that build process. (The fix also resolves 16 existing instances of this problem, so it's great that this PR helped catch them!)

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Tthe Eleventy build process has now been successfully used to push the latest site update, and merged into the main branch, so it's now safe to merge this PR.

@mbgower mbgower merged commit 5d51bfb into main Jul 25, 2024
1 check passed
@mbgower mbgower deleted the patrickhlauke-fix-dfns branch July 25, 2024 00:30
Copy link

netlify bot commented Jul 25, 2024

Deploy Preview for wcag2 failed.

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/wcag2/deploys/66a27651a246335e1b8ec0dd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some definition links (#dfn-*) point to fully-qualified URLs
4 participants