892817 - [META] Slideout plugin

Status: RESOLVED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Matthew Schranz [:mjschranz]]

Technical requirements notes:

• Graphical redesign and css update to the style of the plugin template
• Minor editor updates if necessary for latest version of butter compliance and support
• This plugin does not need to be supported in IE8

The basics of this plugin are already completed. It will mostly be updating work.

Comment 1

[Matthew Schranz [:mjschranz]]

Work that was done with Fivel previously ported over to this repo.

https://github.com/mjschranz/popcorn.webmaker.org/tree/slider

Comment 2

[Kieran Sedgwick (:sedge)]

Known issues:

1) Inability to drag (and resize?)
2) Add a field called "Themes" (templates?)
3) Default size is waayy too big

Comment 3

[Kieran Sedgwick (:sedge)]

4) Ability to add a link to a section of text (only once)

Comment 4

[Kieran Sedgwick (:sedge)]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Functionality is complete, styling left to be done.

Comment 5

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Reviews comments.

Lint:
public/templates/assets/editors/slider/slider-editor.js: line 61, col 41, Missing semicolon.

Comment 6

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

:mjschranz - Fixed nits, just need that r+

:humph - its good to go now, just need a secondary review

Comment 7

[David Humphrey (:humph)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

I'm probably not a good reviewer for this type of change.  Moving to Scott.

Comment 8

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

The descriptions text can leak out before it hits the max count of characters we provide.

Comment 9

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

mmmmm, how about....now?

Comment 10

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

I'm really sorry Kieran but we forgot something. In the Editor (Popcorn Maker) we don't want someone clicking that anchor tag to pause the video and then open the link in a new window.

We do this with the text and popup plugins, for example https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/editors/text/text-editor.js#L164-L166 https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/editors/text/text-editor.js#L144-L149

Comment 11

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Woo!

Comment 12

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

R+ with the one comment in the PR. Needs a duplicate file removed.

Comment 13

[Kieran Sedgwick (:sedge)]

From Scott's review:

1) Remove tooltip

Add optional tooltip logic to EditorHelper object (https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/editors/editorhelper.js#L151)

2) Add localization keys for SliderEditor fields 'n stuff

3) From Scott: "if I drop a slider track event on the timeline, and select it on the timeline, then deselect it, I can see it move up and down one pixel in the stage as I select a deselect it."

Possible fix? 
"if you add a margin: -1px; to the popcorn-slider-light element it stops it from moving up and down."

Comment 14

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Again!  Post-review fixes..

Comment 15

[Scott [:thecount] Downe]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

It is a little weird the contents title is the same as the default text, seems redundant to me, but is not a big deal. Going to pass this and you can decide if it is redundant or not.

Comment 16

[Kieran Sedgwick (:sedge)]

What do we do now?

Comment 17

[Matthew Schranz [:mjschranz]]

I handle getting this landed in Fivel's Popcorn Maker, and we figure out what we want to do with this plugin in regards to it's use with Webmaker's Popcorn Maker.

Comment 18

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Code review, + testing please!

Comment 19

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Comments in pull request.

Also, we need to add this plugins LESS file to our linting. See how it's done with https://github.com/mozilla/popcorn.webmaker.org/blob/master/make.js#L99-L108

Comment 20

[Matthew Schranz [:mjschranz]]

Also lint errors:

public/templates/assets/plugins/slider/popcorn.slider.js: line 107, col 8, Missing semicolon.
public/templates/assets/plugins/slider/popcorn.slider.js: line 118, col 6, Missing semicolon.

Comment 21

[Kieran Sedgwick (:sedge)]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

Next pass

Comment 22

[Matthew Schranz [:mjschranz]]

Comment on attachment 786465 [details]
https://github.com/mozilla/popcorn.webmaker.org/pull/150/files

All good, but one suggestion.

Add and editable property to the options object here https://github.com/mozilla/popcorn.webmaker.org/pull/150/files#L5R11 set to false. This way we don't trigger the double click event on the container.

Comment 23

[Matthew Schranz [:mjschranz]]

Only certain people will be able to see this, but for record keeping sake this is the current pull request open for this plugin with Fivel https://github.com/Fivel/butter/pull/2