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

Screen Reader Accessibility #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jpdevries
Copy link

@jpdevries jpdevries commented Nov 12, 2016

Closes #4

Adds a semantic heading structure making the app navigable by the VoiceOver rotor

Semantic form elements provide implicit accessibility by being navigable by the rotor as well.

Wraps the list section of the app in a form to encourage progressive enhancement.

The previous check and delete button weren't semantic or accessible. An <input type=checkbox> is the semantic way to toggle something like completion and should be used even if you opt to use CSS to hide the checkbox visually. An example of this would be Makeanico which uses a <input type=checkbox> for each cell of the art board.

Rather than a delete button, in the spirit of progressive enhancement checkboxes are used for the delete feature. The idea is that if JS breaks you check the items you want to delete and click the submit or trash deleted items button, it hit the server, and does so. Even if JS works, this is a nice features because it allows the user to undo an accidental deletion without having to ask for confirmation each time.

Uses the .visually-hidden pattern from a11y-project to hide elements visually but keep them available to screen readers. This is useful so the delete items read "delete milk" rather than just "delete".
http://a11yproject.com/posts/how-to-hide-content/

Uses the :target pseudo selector so for really long lists you can add #walnuts to the end of the URL and scroll half way down the list to that item to find that item highlighted.

Srikethrough complete styles do not depend on JavaScript (notice milk is initially checked semantically with <input checked>).

@jpdevries
Copy link
Author

If this is merged I can submit a PR to Thinkful-Ed/shopping-list-solution also

Closes Thinkful-Ed#4

formatting CSS

no-js friendly strikethrough feature

Making milk checked initially
</ul>
<form id="js-shopping-list-form">
<ul class="shopping-list">
<li id="apples">
Copy link

@mxmason mxmason Feb 2, 2017

Choose a reason for hiding this comment

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

Would a data-* attribute not be a better way to name each list item?

I feel like it would be if we didn't need unique IDs later for the form elements. Given this need, it's a good idea to use underscores to indicate states, etc., but I wanted to bring this up. Thoughts?

Copy link
Author

@jpdevries jpdevries Feb 6, 2017

Choose a reason for hiding this comment

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

@dengeist I think the advantage of using an ID is they double as a hash jump to link to scroll to and :target that element. In other words, the id attribute isn't just for CSS

Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying issue here is that we don't want to lock the student into a given way of managing the state for this. If we have state being stored in ids or in data attributes in the template, they're just going to use that approach when they create their app.

@jpdevries can you please explain the reasoning behind the ids?

Copy link

Choose a reason for hiding this comment

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

Good point about offering too many opinions, @benjaminEwhite, but isn't this also a good time to teach the data-*attribute? This is a great use-case for it.

Copy link
Author

Choose a reason for hiding this comment

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

@benjaminEwhite the id isn't for managing state. it's for accessible jump-to :target links Uses the :target pseudo selector so for really long lists you can add #walnuts to the end of the URL and scroll half way down the list to that item to find that item highlighted.

Uses the :target pseudo selector so for really long lists you can add #walnuts to the end of the URL and scroll half way down the list to that item to find that item highlighted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's nifty.

@@ -13,58 +13,57 @@
<div class="container">
<h1>Shopping List</h1>

<form id="js-shopping-list-form">
<label for="shopping-list-entry">Add an item</label>
<form id="js-shopping-list-add-item">
Copy link
Contributor

Choose a reason for hiding this comment

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

The js- prefix is reserved for ids and classes that are to be targeted by JS layer. Shouldn't be used for CSS. The one that's in master right now should actually be gone altogether, so if you need an id here for CSS, can create new one.

Copy link
Author

Choose a reason for hiding this comment

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

@benjaminEwhite just loosened the CSS selector to not reference the js id

</ul>
<form id="js-shopping-list-form">
<ul class="shopping-list">
<li id="apples">
Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying issue here is that we don't want to lock the student into a given way of managing the state for this. If we have state being stored in ids or in data attributes in the template, they're just going to use that approach when they create their app.

@jpdevries can you please explain the reasoning behind the ids?

main.css Outdated
@@ -18,6 +30,13 @@ button:hover {
width: 250px;
}

#js-shopping-list-add-item h2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

see note above about js- prefix.

}

.shopping-item__checked {
input[type="checkbox"]:checked + label {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@benjaminEwhite
Copy link
Contributor

What happens when you have multiple of same item on list and end up with doubled id values?

@benjaminEwhite
Copy link
Contributor

@jpdevries finally grokked what this is doing. cool stuff.

So, in short term, not ready to merge this for v6 of FEWD because we need additional, earlier content in curriculum to establish the whys/hows of using targets + ids for creating navigable UIs.

In medium term, i think that this is the right way to go directionally. I've started designing FEWD v7, and a11y/progressive enhancement are must have features. Seems to me that we could add a reading to Unit 1 Lesson 6 that focuses on creating interactive UIs with just HTML and CSS. The lesson could be tweaked a bit so it's "Basic user interfaces". It would still teach forms as it does now, but we'd go deeper and explore some of the ways you can create robust interfaces without JS.

@jpdevries
Copy link
Author

What happens when you have multiple of same item on list and end up with doubled id values?

ids have to be unique so sometimes I'll append the items unique id so something like walnut__5

@jpdevries
Copy link
Author

@benjaminEwhite

So, in short term, not ready to merge this for v6 of FEWD because we need additional, earlier content in curriculum to establish the whys/hows of using targets + ids for creating navigable UIs.

If it would be helpful to pull the targets + ids stuff out for now I can. Otherwise I'll assume we'll hold off for FEWD 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants