Inconsistent formatting of text with octicons

akordowski commented 4 days ago

Code of Conduct

[X] I have read and agree to the GitHub Docs project's 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.

nguyenalex836 commented 3 days ago

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

subatoi commented 3 days ago

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:

It'd be ideal if you could limit your changes to different sections of the content at a time rather than all at once, to limit the risks of merge conflicts on a large scale. That is, if you could open multiple PRs.
To your second point, this isn't necessarily a discrepancy (see https://docs.github.com/en/contributing/writing-for-github-docs/using-markdown-and-liquid-in-github-docs#octicons). So I'd prefer to limit any changes to the positioning of ** (boldface)

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

akordowski commented 3 days ago

@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 commented 3 days ago

@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 commented 3 days ago

@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 commented 3 days ago

@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 commented 3 days ago

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

github-actions[bot] commented 3 days ago

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

nguyenalex836 commented 2 days ago

@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 commented 2 days ago

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

https://github.com/github/docs/blob/17af2826c8d29cc01245cb46a35cbb4fffc2c093/content/actions/managing-workflow-runs-and-deployments/managing-deployments/managing-environments-for-deployment.md?plain=1#L209-L212

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

nguyenalex836 commented 2 days ago

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

nguyenalex836 commented 2 days ago

@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! ✨

akordowski commented 2 days ago

@nguyenalex836 Thank you, will do tomorrow.

github / docs