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

Radio documentation: updating radio readme #11226

Merged
merged 12 commits into from
Dec 21, 2018
Merged

Conversation

davewhitley
Copy link
Contributor

@davewhitley davewhitley commented Oct 29, 2018

Initial commit to update Radio readme. These changes add design documentation.

Description

This PR adds design guideline documentation in addition to the development documentation that existed previously. These guidelines are 'best practices' for the usage of the component, as well as describing the component in more detail.

Screenshots

A preview can be seen here, although it doesn't represent what it will look like in the handbook:

http://simp.ly/p/qzstJq

Feedback

I'd appreciate feedback on:

  • the image alt text
  • usage of the h1 header
  • design guidelines
  • finding a way to preview these changes and see what they'll look like in the handbook

Thank you!

Initial commit to update Radio readme. These changes add design documentation.
@davewhitley davewhitley added the [Status] In Progress Tracking issues with work in progress label Oct 29, 2018
Updating the path of the images. Making a small tweak to phrasing.
@davewhitley
Copy link
Contributor Author

davewhitley commented Oct 30, 2018

Thanks for the review @chrisvanpatten. I would love some a11y guidance on the alt text and use of headers.

I wasn't sure about how to write alt text for an image with a visible caption. I know some sites use the same caption text for the alt text, but make sure that it's not read twice.

Sounds like next steps for this would be:

  • Update images to be consistent with Joen's new do/don't images
  • Determine best way forward for alt text on these images
  • Figure out if there should only be a single h1 on the page

@davewhitley davewhitley added [Feature] UI Components Impacts or related to the UI component system [Type] Developer Documentation Documentation for developers Needs Accessibility Feedback Need input from accessibility and removed [Status] In Progress Tracking issues with work in progress labels Oct 30, 2018
@davewhitley
Copy link
Contributor Author

I just read this to get a better handle on alt text and captions: https://webaim.org/techniques/alttext/

I'm still not sure, but leaving the alt text blank might be a valid approach because of the surrounding text.

Another possibility is describing what is going on in the diagram. So, for example, this image:

screen shot 2018-10-30 at 11 27 55 am

the alt text could be:

"A diagram of an interface using checkboxes when only one item can be selected from a list."

@davidakennedy
Copy link

I would love some a11y guidance on the alt text and use of headers.

Hi @drw158! Happy to share some guidance. Love that you're thinking about this, and asking great questions. ❤️

First, for the headings:

  • Ideally, there should be one h1 on a page. Why? Because screen reader users often scan a page by heading levels, much like sighted users skim a page visually looking for the headlines. Having one h1 helps them know – this is the page title, the topic of the page. An added benefit is SEO. One h1 has some benefits there as well.
  • Even though HTML5 supports a document outline that can include multiple h1s in a page, it's not supported by any browser, so it's not practical to rely on.

For the alt attributes:

  • What I do in this case is imagine that the images aren't there. Would the content lose any value? In this case, no. As you say, the content covers the same guidelines that the second and third images show. It's fine to use a blank alt attribute on those.

@samikeijonen
Copy link
Contributor

I agree with @davidakennedy in the previous comment.

Although I can't really see the context of the image and surrounding text. If alt is needed it could be like "Post visibility checkboxes: public, private, and password protected."

@drw158 Any change to update the PR?

@davewhitley davewhitley removed the Needs Accessibility Feedback Need input from accessibility label Nov 28, 2018
The alt text for these images has been removed because removing the images from the document doesn't affect comprehension.
@davewhitley
Copy link
Contributor Author

@samikeijonen Can you explain what you mean when you say you can't see the context of the image?

screen shot 2018-11-28 at 2 56 10 pm

@chrisvanpatten all of your concerns should be fixed


I'm not sure where the images should be uploaded. That is the only blocker right now.

@davewhitley
Copy link
Contributor Author

Image issue has been resolved. Now ready for a final review and merge.

@sarahmonster sarahmonster requested a review from a team December 21, 2018 11:40
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This is an improvement so I'm gonna say let's go for it.


When using radio buttons **one should be selected by default** (i.e., when the page loads, in the case of a web application).

**User control**
Copy link
Member

Choose a reason for hiding this comment

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

These are headers, not paragraph text; they should be labelled with header markdown (eg ##### User control), not bold text like this. 😄

@tofumatt tofumatt dismissed chrisvanpatten’s stale review December 21, 2018 15:41

Issues were addressed

Copy link
Contributor

@chrisvanpatten chrisvanpatten left a comment

Choose a reason for hiding this comment

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

Looks good to me and I'm glad we're using make/design for image uploads!

@tofumatt tofumatt merged commit 3f23b1a into master Dec 21, 2018
@tofumatt tofumatt deleted the update/radio-readme branch December 21, 2018 15:56
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Updating Radio readme

Initial commit to update Radio readme. These changes add design documentation.

* Adding readme images

* Image path update

Updating the path of the images. Making a small tweak to phrasing.

* Updates so that only a single h1 is present

* image alt text removal

The alt text for these images has been removed because removing the images from the document doesn't affect comprehension.

* Removing images

* Removing images

* Removing images

* Adding images hosted on wordpress.org

* docs: tweak some usage recommendations

* Tweak spacing

* docs: Use headers
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Updating Radio readme

Initial commit to update Radio readme. These changes add design documentation.

* Adding readme images

* Image path update

Updating the path of the images. Making a small tweak to phrasing.

* Updates so that only a single h1 is present

* image alt text removal

The alt text for these images has been removed because removing the images from the document doesn't affect comprehension.

* Removing images

* Removing images

* Removing images

* Adding images hosted on wordpress.org

* docs: tweak some usage recommendations

* Tweak spacing

* docs: Use headers
@kjellr kjellr mentioned this pull request Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants