Skip to content

Improve screen reader user experience on logistration page - #8699

Merged
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:sso-a11y
Jun 30, 2015
Merged

Improve screen reader user experience on logistration page#8699
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:sso-a11y

Conversation

@bradenmacdonald

Copy link
Copy Markdown
Contributor

Description
This is a follow-up to #8603, addressing @cptvitamin's feedback to improve the experience of screen reader users on the logistration page.

Merge Target: Cypress release

Screenshot: Looks visually identical to #8603

Sandbox: http://sandbox4.opencraft.com/login (Updated June 29 to 49d200c)

Reviewers:
@Kelketek and @cptvitamin

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! I've created OSPR-674 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ('this must be merged by XX date', and why that is)
  • partner information ('this is a course on edx.org')
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

@Kelketek

Copy link
Copy Markdown
Contributor

👍

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@cptvitamin This is ready for your review.

@sarina FYI - you wanted this before Cypress.

@sarina

sarina commented Jun 29, 2015

Copy link
Copy Markdown
Contributor

Yep, I tagged Mark on the JIRA ticket, if he doesn't review by Wednesday I'll bug him in person to make sure we get it merged by Thursday afternoon.

@cptvitamin

Copy link
Copy Markdown
Contributor

This is great @bradenmacdonald 👍 !!
cc @sarina

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

Thanks @cptvitamin !

Unfortunately, this isn't quite ready for merge. I just noticed on the sandbox that the third party auth buttons are not working now if I click on certain parts of the button. Must be an issue with propagation of JS event handlers. I will look into it ASAP.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

Ok, I just amended this PR to include this simple fix: 47bae62fbdc42684f9f5febc7badfc19d5f2d4f4

Since I added a new <span> element to each of the third party provider buttons, the existing click event code was looking for the URL to the third party provider on the <span> instead of the <button>'s data attributes. By using currentTarget instead of target in the JS, we get the attribute from the element on which the event was registered.

@Kelketek Can you please just review the diff I linked above and confirm on the updated sandbox that the third party buttons work, before I merge?

@Kelketek

Copy link
Copy Markdown
Contributor

@bradenmacdonald The buttons are certainly clickable. The configuration for Facebook and Google appears to be bad, but that's not actually something this PR is testing, so 👍

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@Kelketek Yeah don't worry about that, I just didn't bother registering API keys with those services for the sandbox. Thanks for checking again.

bradenmacdonald added a commit that referenced this pull request Jun 30, 2015
Improve screen reader user experience on logistration page
@bradenmacdonald
bradenmacdonald merged commit e145d7f into openedx:master Jun 30, 2015
@bradenmacdonald
bradenmacdonald deleted the sso-a11y branch June 30, 2015 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants