-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Improve accessibility of the docs #9338
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
This update refactors the `linkifyAnchors` function to use arrow function syntax and the `Array.from()` method to convert the result of `getElementsByTagName()` to an array. The update also uses template literals to clean up the anchor.href assignment. Additionally, `var` declarations are replaced with `const` or `let`.
Revised liquid syntax for navigation menu with additional variables, reducing lines of code.
The change involves removing unnecessary hyphen in the `for` loops, but otherwise retains the original functionality of the navigation.
- Added aria-hidden="true" to the i tag to indicate that it doesn't provide any important information for assistive technologies
The language attribute "en-US" is not necessary because "en" is sufficient to indicate the document's language as English, and any regional variations can be conveyed through other means such as regional dialect or locale-specific settings.
Both <!DOCTYPE html> and <!DOCTYPE HTML> are valid ways to declare the document type for HTML5 documents. However, it's recommended to use <!DOCTYPE html>
Added defer attribute to script for faster page load.
- added accessibility to mobile navigation - moved navigateToUrl to footer
…html - Converted inline comments from single-line (//) to block style (/* */). - Amended double quotation marks in news_item_archive.html to rectify an error.
Resolved Previous Issues and Awaiting Test Results.
|
It appears that the issue has been successfully resolved. This marks a significant improvement in accessibility, complemented by schema integration. |
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.
Would it be possible to break this down further? Some things are good, like the improved accessibility, the better javascript, etc. However, it's still too wide of a PR to be merged as is.
Ideally, I'd see something like two or three PRs for this.
- Javascript Improvements
- Accessibility Improvements
- Enhanced schema usage for the docs site
The JS and accessibility improvements look like they could go hand in hand and if it makes more sense to combine those two things into a single PR, I think that's fine.
{% for section in site.data.docs_nav %} | ||
<optgroup label="{{ section.title }}"> | ||
{%- for item in section.docs -%} | ||
{% assign page = site.docs | where: "url", item.link | first %} | ||
<option value="{{ page.url | relative_url }}"> | ||
{{- page.menu_name | default: page.title -}} | ||
</option> | ||
{%- endfor %} | ||
</optgroup> | ||
{% endfor %} |
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'm not following the reasoning for the re-indentation work in this PR. We've not indented the liquid in most pages because it's not present in the final site HTML, where as indenting it along with the HTML gives disjointed indentation in the final output.
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.
Hey @mattr-, I'm not sure either but I'm going to assume that the indentation was modified exclusively to improve readability in my editor. Given that this is a substantial pull affecting numerous files, I reckon we maintain it as is for now with the possibility of removing it later (?). What do you think?
Co-authored-by: Matt Rogers <mattr-@github.com>
Co-authored-by: Matt Rogers <mattr-@github.com>
Absolutely, I concur. I'm planning additional enhancements in a separate PR, so your recommendations are helpful as I'd like to make it as easy as possible for us to review it. Could we proceed with merging this one, and then for the subsequent updates, I'll implement the splitting as recommended? Your input would be appreciated. |
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.
Yeah, I think we can get this merged for now. Thanks for improving our accessibility. ❤️
@jekyllbot: merge +docs
Daniel Haim: Improve accessibility of the docs (#9338) Merge pull request 9338
🎉 |
@danielhaim1 This PR introduced #9527. Reverting it fixes it. #9529 attempts to fix the issue by just reverting one file, but does not fix it. |
Merge pull request 9338
Summary
Improvements made to the docs include changing
<!DOCTYPE HTML>
to<!DOCTYPE html>
anden-US
toen
, adding missing accessibility, documentation and aria-labels, implementing structured data to templates, addingtarget="_blank"
to some external links, and addingdefer
attribute for faster page load.Still making improvements, but I realize I should start chunking these updates down so that the team can review them more efficiently, and to reduce their workload.
This is a 🐛 bug fix.
This is a 🙋 feature or enhancement.
This is a 🔨 code refactoring.