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

Improve accessibility of the docs #9338

Merged
merged 17 commits into from Jan 12, 2024
Merged

Conversation

danielhaim1
Copy link
Contributor

Summary

Improvements made to the docs include changing <!DOCTYPE HTML> to <!DOCTYPE html> and en-US to en, adding missing accessibility, documentation and aria-labels, implementing structured data to templates, adding target="_blank" to some external links, and adding defer 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.

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.
@danielhaim1
Copy link
Contributor Author

Resolved Previous Issues and Awaiting Test Results.

  • Converted inline comments from single-line (//) to block style (/* */).
  • Amended double quotation marks in news_item_archive.html to rectify an error.

@danielhaim1
Copy link
Contributor Author

It appears that the issue has been successfully resolved. This marks a significant improvement in accessibility, complemented by schema integration.

Copy link
Member

@mattr- mattr- left a 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.

docs/_includes/anchor_links.html Outdated Show resolved Hide resolved
docs/_includes/anchor_links.html Outdated Show resolved Hide resolved
Comment on lines +4 to +13
{% 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 %}
Copy link
Member

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.

Copy link
Contributor Author

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?

danielhaim1 and others added 2 commits January 6, 2024 18:26
Co-authored-by: Matt Rogers <mattr-@github.com>
Co-authored-by: Matt Rogers <mattr-@github.com>
@danielhaim1
Copy link
Contributor Author

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.

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.

@mattr- mattr- changed the title docs/a11y + refactoring Improve accessibility of the docs Jan 12, 2024
Copy link
Member

@mattr- mattr- left a 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

@jekyllbot jekyllbot merged commit 55e993b into jekyll:master Jan 12, 2024
11 checks passed
jekyllbot added a commit that referenced this pull request Jan 12, 2024
github-actions bot pushed a commit that referenced this pull request Jan 12, 2024
Daniel Haim: Improve accessibility of the docs (#9338)

Merge pull request 9338
@danielhaim1
Copy link
Contributor Author

Yeah, I think we can get this merged for now. Thanks for improving our accessibility. ❤️

@jekyllbot: merge +docs

🎉

@heironimus
Copy link

@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.

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.

None yet

5 participants