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

WIP A11y updates #87

Closed
wants to merge 15 commits into from
Closed

WIP A11y updates #87

wants to merge 15 commits into from

Conversation

jenweber
Copy link
Contributor

@jenweber jenweber commented Dec 11, 2017

WIP: once this PR is approved, I'll do the work in the tutorials app. Once both repos have PRs, they can be merged

Addresses #86. This PR reworks some of the changes made in #26

I have addressed the CSS errors. Todd, I reverted some of the work on roles because I couldn't resolve the warnings. I think certain role elements should be direct children/siblings and this app doesn't work that way right now. I'd rather show a simpler implementation that introduces a11y concerns but doesn't confuse new devs.

CSS changes are as slight as possible to possibly avoid redoing every single screenshot. Opinions on whether it's close enough?

cc @toddjordan @MelSumner

@jenweber
Copy link
Contributor Author

My PR styling:
screen shot 2017-12-10 at 7 20 51 pm

Old PR styling:
screen shot 2017-12-10 at 7 22 37 pm

@@ -1,6 +1,6 @@
<article class="listing">
<a {{action 'toggleImageSize'}} class="image {{if isWide "wide"}}">
<img src="{{rental.image}}" alt="">
<a {{action 'toggleImageSize'}} class="image {{if isWide "wide"}}" aria-hidden="true" tabindex="-1">
Copy link
Member

Choose a reason for hiding this comment

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

  • Interactive elements are not supposed to be hidden- can you tell me what the purpose would be here?
  • Also, if you were to hide an element, you wouldn't need an alt attribute value on a child element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was overlooked during copy & paste. I'll fix this and then get started on any edits to the tutorial

@jenweber jenweber changed the title A11y updates WIP A11y updates Dec 26, 2017
@toddjordan toddjordan dismissed MelSumner’s stale review March 31, 2019 03:32

concern addressed

Copy link
Contributor

@toddjordan toddjordan left a comment

Choose a reason for hiding this comment

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

Alright, a few of things before merging this one. First, I resolved conflicts, but then had to fix a couple of things. I pr'd those updates to your branch: https://github.com/jenweber/super-rentals/pull/1

Second, the changes you made to the css files need to go back into tutorial styles addon. I see you made a pr so I will take care of that.

Third, we need to update the tutorial. Its all you if you have bandwidth. Otherwise I can take it on.

@toddjordan
Copy link
Contributor

@jenweber The travis errors will go away once https://github.com/jenweber/super-rentals/pull/1 is merged

@toddjordan
Copy link
Contributor

closing this one in favor of #121 (conflict resolutions, test fixes)

@toddjordan toddjordan closed this Apr 4, 2019
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