[meta] Keyboard Interactions Plugin

RESOLVED FIXED

Status

Webmaker
Popcorn Maker
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mjschranz, Assigned: sedge)

Tracking

Details

(Whiteboard: fivel-bugs)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 775858 [details]
https://github.com/mjschranz/popcorn.webmaker.org/tree/interaction

WIP patch
(Assignee)

Updated

4 years ago
Summary: Keyboard Interactions Plugin → [meta] Keyboard Interactions Plugin
(Assignee)

Comment 2

4 years ago
Created attachment 799070 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/192
Attachment #799070 - Flags: review?(schranz.m)
(Reporter)

Updated

4 years ago
Attachment #775858 - Attachment is obsolete: true
(Reporter)

Comment 3

4 years ago
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
(Assignee)

Comment 4

4 years ago
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)
(Assignee)

Comment 5

4 years ago
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-
(Assignee)

Comment 7

4 years ago
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-
(Reporter)

Comment 9

4 years ago
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-
(Assignee)

Comment 10

4 years ago
Created attachment 831531 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/315

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)
(Assignee)

Comment 11

4 years ago
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)
(Assignee)

Comment 12

4 years ago
:humph this part specifically: https://github.com/sedge/popcorn.webmaker.org/blob/24e973f6b675cece4f8415dd146c3ff3582832ed/public/templates/assets/plugins/interaction/popcorn.interaction.js#L12-297
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-
(Assignee)

Comment 14

4 years ago
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)
(Assignee)

Comment 15

4 years ago
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)
(Reporter)

Comment 16

4 years ago
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-
(Reporter)

Comment 17

4 years ago
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.
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.