Skip to content

Badges Share Modal - #11028

Merged
Kelketek merged 1 commit into
feature/badges-v2from
kelketek/badges-share-modal
Dec 30, 2015
Merged

Badges Share Modal#11028
Kelketek merged 1 commit into
feature/badges-v2from
kelketek/badges-share-modal

Conversation

@Kelketek

Copy link
Copy Markdown
Contributor

Description: Provides a share button and modal that allows students to share their achievements from their profile page.
Dependencies: https://github.com/edx/edx-platform/pull/10861 https://github.com/edx/edx-platform/pull/11078
Jira: https://openedx.atlassian.net/browse/SOL-1456
Sandbox: http://pr10556.sandbox.opencraft.com/dashboard

@bradenmacdonald Only the last commit in this set is relevant-- the rest is dependencies. @talbs For you as well.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=11028

@Kelketek Kelketek mentioned this pull request Dec 18, 2015
@Kelketek Kelketek changed the title (WIP) Badges Share Modal Badges Share Modal Dec 18, 2015
@Kelketek Kelketek mentioned this pull request Dec 18, 2015
6 tasks
@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 7d0b40b to 96093fe Compare December 18, 2015 05:31
@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Could you give this an a11y look over today? Just the last commit-- the previous ones are dependencies.

@cptvitamin

Copy link
Copy Markdown
Contributor

@Kelketek i did a quick code review but didn't do a manual review. I checked the sandbox and couldn't immediately figure out how to access the view. I'm on my way out the door, and out next week, but if you can give me explicit instructions for how to do a manual review, I can squeeze it in ASAP. Best.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Log in as the honor user and go to the honor user's profile, to the accomplishments tab, and click the share button.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Addressed your notes so far. The images that needed alt attributes have been given blank ones, since they are decorative/illustrative but the text instructions are comprehensive.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@Kelketek Looks good! I had a bunch of minor comments - hopefully none of them are too involved. One last request: Clicking outside the modal or pressing escape should close the modal.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from ba5ae6e to 6d912a0 Compare December 21, 2015 18:04
@Kelketek

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald This is ready for you to give another look.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 6d912a0 to 67f12f4 Compare December 21, 2015 18:39
@talbs

talbs commented Dec 21, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, thanks for the ping. The UI is generally fine for the Open Badges needs. I noticed a few small visual aspects that should be cleaned up.

modal

  • Spacing on the modal header/title is off
  • There is no bottom whitespace between the modal's content and the end of the modal window itself. This makes it feel like content is cut-off.
  • Is there a min-width for the modal or the items contained in it? At smaller screens, the contents really get truncated.

Once you address the items above and @cptvitamin is fine with this from an a11y-perspective, you have my 👍


NOTE: I did not review the HTML nor the Sass/CSS with this work. if you need feedback on that, someone in Engineering can be assigned.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 67f12f4 to ea8b03b Compare December 21, 2015 19:42
@Kelketek

Copy link
Copy Markdown
Contributor Author

@talbs I've updated the CSS-- I think this should satisfy the issues you raised.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 2858f50 to 7341c6c Compare December 21, 2015 20:53
@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 7341c6c to 562b3b3 Compare December 21, 2015 21:25
@bradenmacdonald

Copy link
Copy Markdown
Contributor

Thanks @Kelketek. Please add an event handler so that pressing ESC closes the modal, then this is 👍 from me.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 562b3b3 to b7a5693 Compare December 22, 2015 19:01
@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from b7a5693 to 922bd01 Compare December 22, 2015 19:54
@cptvitamin

Copy link
Copy Markdown
Contributor

@Kelketek Thanks for incorporating these changes. When I performed a manual test, I noticed that there are some issues with the modal dialog box. The first issue is that focus is not moved to the dialog box once it is opened. A keyboard only or screen reader user's focus remains on the Share button. Subsequent TAB presses move them to the next Share button, not to the the instructions inside the modal dialog box. Once focus is moved to the dialog box, the focus should be trapped within the dialog, just like the behavior you would expect from an OS dialog box. That means you cannot tab outside the modal, but either remain on the last focusable item, or cycle back to the beginning of the modal. Support for closing the modal dialog with the ESC key should also be added. Closing the modal should return focus to the button that opened it. All of this is covered in our Accessibility Guidelines for Developers document.

A couple of small things worth mentioning: the instructions to download the badge image refer to "right-click". I would say "option-click". Also, the accessible text for the Share buttons are identical, which makes them indistinguishable from each other when viewed out of context. This is what a screen reader user hears when accessing all form fields out of context:
screenshot of repeating share button values

As you can see, all of the share buttons have identical text. Instead of using "Mozilla Open Badges Backpack" as the alt text for the image, can you add the badge title itself instead? e.g. "Share your Batman Award Badge"

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Alright. I think this should resolve the last items you gave me. The sandbox has been updated. Could you take another look?

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 1afb2f4 to 7517dc4 Compare December 29, 2015 14:02
@cptvitamin

Copy link
Copy Markdown
Contributor

@Kelketek There is no visual focus indicator on the Close button (as a sighted keyboard only user I would have no idea that the close button has focus) and I can still TAB beyond the modal. When focus is on the "upload" link, tabbing should cycle focus back to the Close button, and vice versa. Another way to demonstrate this is by pointing out that a mouse user cannot click on any of the buttons on the page when the dialog box is open, but a keyboard user can.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Is tab-constraining functionality implemented elsewhere in the codebase? Is there a standard way to do this? I imagine I'm not the first and I don't want to re-invent the wheel (again) which cost me a great deal of time earlier in this project.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cahrens @andy-armstrong Perhaps one of you might know where code is that allows one to isolate tabbing to a modal?

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin I found a way to do this which wasn't too complicated. Thanks to @bradenmacdonald for the tip. Could you please take another look?

@cptvitamin

Copy link
Copy Markdown
Contributor

@Kelketek did you happen to update the sandbox? I'm not seeing the focus indicator on the close button or the trapped focus.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@cptvitamin Sorry about that. Should be ready now.

@cptvitamin

Copy link
Copy Markdown
Contributor

👍 nice work. Thanks for adding those accessibility improvements.

@Kelketek
Kelketek force-pushed the kelketek/badges-share-modal branch from 2d5d061 to 8fb1814 Compare December 30, 2015 22:24
@Kelketek
Kelketek merged commit 8fb1814 into feature/badges-v2 Dec 30, 2015
@Kelketek
Kelketek deleted the kelketek/badges-share-modal branch December 30, 2015 22:25
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.

5 participants