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

Inconsistent formatting of text with octicons #35326

Closed
1 task done
akordowski opened this issue Nov 18, 2024 · 13 comments · Fixed by #35376
Closed
1 task done

Inconsistent formatting of text with octicons #35326

akordowski opened this issue Nov 18, 2024 · 13 comments · Fixed by #35376
Labels
content This issue or pull request belongs to the Docs Content team SME reviewed An SME has reviewed this issue/PR

Comments

@akordowski
Copy link
Contributor

akordowski commented Nov 18, 2024

Code of Conduct

What article on docs.github.com is affected?

Multiple articles.

What part(s) of the article would you like to see updated?

I noticed that the formatting of text with octicons is in the docs inconsistent:

{% octicon "pencil" aria-hidden="true" %} **Edit**

OR

**{% octicon "pencil" aria-hidden="true" %} Edit**

EDIT:
There is also the usage of aria-label instead of aria-hidden="true":

**{% octicon "gear" aria-label="The Settings gear" %} Settings**

The formatting should be unified to:

**{% octicon "pencil" aria-hidden="true" %} Edit**

Additional information

I could provide a PR to fix this issues.

@akordowski akordowski added the content This issue or pull request belongs to the Docs Content team label Nov 18, 2024
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Nov 18, 2024
@nguyenalex836 nguyenalex836 added waiting for review Issue/PR is waiting for a writer's review and removed triage Do not begin working on this issue until triaged by the team labels Nov 18, 2024
@nguyenalex836
Copy link
Contributor

@akordowski Thank you for raising this issue! I'll get this triaged for review ✨ Our team will provide feedback regarding the best next steps for this issue - thanks for your patience! 💛

@subatoi
Copy link
Contributor

subatoi commented Nov 19, 2024

Hi @akordowski—many thanks for offering to open a PR for this and for opening an issue first.

To your first point: you're correct that this is a discrepancy. Did you run a script to determine how many times one or the other occurs? I'm not aware that this causes any problems with the end user experience with the docs, but in principle, I have no problem opening that up to you to contribute. However, please see my points below:

If all this sounds OK, I'm happy to open it to you to contribute.

@akordowski
Copy link
Contributor Author

@subatoi Thank you for your response.

Did you run a script to determine how many times one or the other occurs?

No, I did a simple text search.

I'm not aware that this causes any problems with the end user experience with the docs

No, it does not and mostly users may not notice it at all 😉 But IMHO a documentation should use a consistent style.

That is, if you could open multiple PRs.

No problem.

To your second point, this isn't necessarily a discrepancy

There are only 22 results of using the aria-label="..." notation, mostly the aria-hidden="true" notation is used. That shows me that the other notation is prefered and that it is not that important to provide a icons description text for text readers. So if it is OK for the team I could provide changes for that as well.

@subatoi
Copy link
Contributor

subatoi commented Nov 19, 2024

@akordowski No problem, thank you for your help! We really appreciate your contributions.

There are only 22 results of using the aria-label="..." notation, mostly the aria-hidden="true" notation is used. That shows me that the other notation is prefered and that it is not that important to provide a icons description text for text readers. So if it is OK for the team I could provide changes for that as well.

I think the bigger picture is that we want to make things as accessible as possible for screen readers, but I'm happy to loop in @nguyenalex836 who can liaise internally with relevant stakeholders and decide on the best way forward for that particular point.

If you prefer to wait for the outcome of Alex's research into that before going ahead with any changes, that's fine ... or else if you prefer, you're welcome to start opening PRs now that are limited to changing the placement of ** characters. Just let me know what you prefer. Many thanks!

@akordowski
Copy link
Contributor Author

@subatoi As requested I provided 4 PRs for the changes addressed in this issue.

If you prefer to wait for the outcome of Alex's research

Yes, of course.

I think the bigger picture is that we want to make things as accessible as possible for screen readers

If this is the case then it would made sense to provide a aria-label="..." description for all octicons.

@subatoi
Copy link
Contributor

subatoi commented Nov 19, 2024

@subatoi As requested I provided 4 PRs for the changes addressed in this issue.

That's great! We'll review those as soon as we can. Thank you so much!

If this is the case then it would made sense to provide a aria-label="..." description for all octicons.

I would agree. I'll let @nguyenalex836 follow up on this point and we'll come back to you separately 👍

@nguyenalex836
Copy link
Contributor

@akordowski @subatoi Thank you both for working on those initial PRs! ✨

I would agree. I'll let @nguyenalex836 follow up on this point and we'll come back to you separately 👍

I'll be reaching out to our SME teams for guidance on this 💛 stay tuned!

@nguyenalex836 nguyenalex836 added the needs SME This proposal needs review from a subject matter expert label Nov 19, 2024
Copy link
Contributor

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert 👀

@nguyenalex836
Copy link
Contributor

@akordowski Hello! 👋 Our SMEs responded with the following when asked about inconsistency regarding our usage of aria-label="..." and aria-hidden="true" notation -

This is for accessibility.

  • We add a label to the octicon ONLY when ithe UI has a symbol and no text. If there was no label then the screen reader would read nothing.
  • If the octicon is just there to supplement a text label, then we hide the octicon from screen readers. They don't need to hear the field label read out twice.

Given the above, I'll go ahead and close this issue as completed since all the linked PRs have been merged 💛 Thank you again for all of your work on this front! ✨

@akordowski
Copy link
Contributor Author

@nguyenalex836 Thanks for the response. But there are also cases where there is an icon AND text, as you can see below:

1. Select the desired option in the **Deployment branches** dropdown.
1. If you chose **Selected branches{% ifversion deployment-protections-tag-patterns %} and tags{% endif %}**, to add a new rule, click **Add deployment branch{% ifversion deployment-protections-tag-patterns %} or tag{% endif %} rule**
{% ifversion deployment-protections-tag-patterns %}1. In the "Ref type" dropdown menu, depending on what rule you want to apply, click **{% octicon "git-branch" aria-label="The branch icon" %} Branch** or **{% octicon "tag" aria-label="The tag icon" %} Tag**.{% endif %}
1. Enter the name pattern for the branch{% ifversion deployment-protections-tag-patterns %} or tag{% endif %} that you want to allow.

For such cases it would make sense to replace the aria-label with aria-hidden="true". What do you think?

@nguyenalex836
Copy link
Contributor

@akordowski No problem at all! Let me check with our SMEs on that scenario, I'll return with more info soon 💛

@nguyenalex836
Copy link
Contributor

@akordowski Our team agrees with you! 💛

For such cases it would make sense to replace the aria-label with aria-hidden="true". What do you think?

Feel free to open PRs for this whenever you have time - thank you! ✨

@nguyenalex836 nguyenalex836 reopened this Nov 20, 2024
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Nov 20, 2024
@akordowski
Copy link
Contributor Author

@nguyenalex836 Thank you, will do tomorrow.

@nguyenalex836 nguyenalex836 added SME reviewed An SME has reviewed this issue/PR and removed triage Do not begin working on this issue until triaged by the team waiting for review Issue/PR is waiting for a writer's review needs SME This proposal needs review from a subject matter expert labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team SME reviewed An SME has reviewed this issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants