Closed Bug 892815 Opened 11 years ago Closed 10 years ago

[meta] Keyboard Interactions Plugin

Categories

(Webmaker Graveyard :: Popcorn Maker, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mjschranz, Assigned: sedge)

Details

(Whiteboard: fivel-bugs)

Attachments

(1 file, 2 obsolete files)

Technical requirements notes:

• This plugin is an extended pause plugin that works identically to the pause plugin with the exception that you can select a keyboard key press combination to advance through the pause and resume the video

• Example: pause and wait for CTRL+C to be pressed. The video may say something like “now you try it” and pause and wait for the user to press the key event

• The plugin should also allow for a specific image to optionally be displayed if it is provided so we can create a poster slide that displays for the user to see (CSS class applied to the div would allow us to set an image as the background)

• The plugin should also allow for specific text to be displayed over the image that we can display to the user. This text may need to be localized in the future and account for support for that.

• This plugin should be supported in IE8
Summary: Keyboard Interactions Plugin → [meta] Keyboard Interactions Plugin
Attachment #799070 - Flags: review?(schranz.m)
Attachment #775858 - Attachment is obsolete: true
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

Comments.

I actually can't run this right now.
Attachment #799070 - Flags: review?(schranz.m) → review-
Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

This is complete relative to my reasonable attempts to fight against technical complications that prevent full functionality. 

This needs testing and thorough code review for style and efficiency would be appreciated!
Attachment #799070 - Flags: review- → review?(schranz.m)
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

Matt knows what to look for in testing, but both feel free to do code review as well - this is likely to be pretty dusty.
Attachment #799070 - Flags: review?(scott)
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

Some initial line comments, but it seems that popcorn.interaction.less never made it into the commit, so I don't see a keyboard.
Attachment #799070 - Flags: review?(scott) → review-
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

I added the LESS file in this time ;)
Attachment #799070 - Flags: review- → review?(scott)
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

Hm, having it on is blocking normal popcorn maker functionality too. This is very confusing. We need to come up with a better idea of whe it goes into input mode. Right now it just goes into input mode on start, but that's too agressive.

Also, still some nits in the pull request.
Attachment #799070 - Flags: review?(scott) → review-
Comment on attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192

Browser defaults still seem to get through.

For example, I register meta+c as a key combo. I tried to meta+t but that opened a new tab rather than blocking it.

If I change the key combo in the editor while it's active with a keycombo already the keyboard slides away.
Attachment #799070 - Flags: review?(schranz.m) → review-
This patch isn't rebased from master because it's going into Fivel's repo.
Attachment #799070 - Attachment is obsolete: true
Attachment #831531 - Flags: review?(schranz.m)
Comment on attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

I'd really value a code review from you Humph, especially since you've worked with keyboard events before.
Attachment #831531 - Flags: review?(david.humphrey)
Comment on attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

This was a big job, now that I read the code.  You really had to deal with a lot of stuff.  Nice work on it.

Matt needs to lead the review of this, since he knows all this code much better than I do. I've made some initial comments on the code in general.  Overall it looks pretty good, just some things you can do to improve it for maintainability.
Attachment #831531 - Flags: review?(david.humphrey) → review-
Comment on attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

I'm gonna push review fixes, then flag you again.
Attachment #831531 - Flags: review?(schranz.m)
Comment on attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

Your turn! Code + testing please
Attachment #831531 - Flags: review?(schranz.m)
Comment on attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

Haven't tested yet, but plenty of comments in the PR now.
Attachment #831531 - Flags: review?(schranz.m) → review-
https://www.dropbox.com/s/4kwglv76hdbkgi2/Screenshot%202013-11-15%2012.54.48.png The keyboard shifts down when the message indicating the correct key sequence is displayed.

Other thoughts considerations:

- Can we UPPERCASE the entire valid key combo? I mean https://www.dropbox.com/s/0i1yalu6ae9821o/Screenshot%202013-11-15%2012.59.15.png so the CMD+E.
- There might be cases where TAB and ENTER are keys that should be part of the sequence but these are both natural ways to update fields/input boxes. As it stands the only way to update a key sequence is to click out of it. Can we change to this to be some sort of APPLY button style?
- For multiple key sequences perhaps we should consider listing them all in the message. Right now it appears to only show the last one registered.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: