1240121 - Convert fxos-media-controls from bower to npm for managing dependencies

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Components, defect)

Description

[Russ Nicoletti [:russn]]

Per Wilson, fxos components and aps should use npm to manage dependencies.

Comment 1

[Russ Nicoletti [:russn]]

Attachment: Github PR

Hi Justin, this is a patch to fxos-media-controls to use npm to manage dependencies instead of bower. 

Also, I made a change for the example demo to resolve an error I saw where the font was failing to load due to the font file not being in the same directory or below the html file [1]. 

Lastly, I added webpack to "browserify" the component (using fxos-header as a model).

1 - https://bugzilla.mozilla.org/show_bug.cgi?id=760436#c1

Comment 2

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8708521 [details] [review]
Github PR

Thanks Russ! LGTM, but I'm gonna pass the feedback? over to Wilson since I haven't been staying current with the latest fxos-components work to know if everything here is alright.

Comment 3

[Wilson Page [:wilsonpage]]

Comment on attachment 8708521 [details] [review]
Github PR

Structure looks good.

- I'd advise adding a "watch" script and "start" script to package.json to make development a little easier [1].

- fxos-icons seem to have been manually copied into the examples/simple. I think this should be managed by NPM. I think perhaps `cd examples/simple`, then `npm install ../.. --save` and then replace the `fxos-media-controls.js` with a symlink to the local copy.

[1] https://github.com/fxos-components/fxos-header/blob/master/package.json#L35

Comment 4

[Russ Nicoletti [:russn]]

Comment on attachment 8708521 [details] [review]
Github PR

Hi Wilson. I've made updates to address your feedback. Regarding the 'fxos-icons' directory that I had added to solve the cross-origin font loading problem, I've removed that directory. I tried your suggestion of 'npm install ../.. --save' in the 'examples/simple' directory but found that updated the 'fxos-media-controls' directory (instead of creating a new 'node_modules' directory, which is what I thought . In any event, loading the example html using a local webserver solves the cross-origin font loading problem. Therefore, I'm concluding it's best for the html to specify the font package by going up the file system to 'fxos-media-controls/node_modules/fxos-icons' instead of creating any new directories and duplicating fxos-icons.

Comment 5

[Wilson Page [:wilsonpage]]

Comment on attachment 8708521 [details] [review]
Github PR

- `npm install` had errors installing `json-wire`. I noticed their is a `preinstall` script in the package.json that may be causing the issue?

- `examples/simple` doesn't work out of the box. I had to manually symlink dependencies into place for it to work.

- The demo and the component should use `fxos-theme`, with the component using fxos-theme variables internally (see other fxos-components as example).

- The pause icon is aligned a little too far to the right, resulting in a jumpy transition from play->pause.

- Buttons don't have active state. I think they should be the same as the buttons in fxos-header and fxos-toolbar, which have a delayed fade-back transition on release.

- There are lots of similarities to this component and fxos-toolbar. I'm wondering if this should extend fxos-toolbar?

- The source contains no comments. I'm particularly interested what the 'contextmenu' event is for :) Is it 'long-press'?

Comment 6

[Russ Nicoletti [:russn]]

Comment on attachment 8708521 [details] [review]
Github PR

Hi Wilson, thanks for your feedback. My updates based on your feedback are described below.

> - `npm install` had errors installing `json-wire`. I noticed their is a `preinstall` script in the package.json that may be causing the issue?

The problem was that marionett-client.1.9.2 sources socket-retry-connect, json-wire-protocol, and sockit-to-me from the local disk. The 1.9.4 version of the package gets those dependencies from the registry and therefore doesn’t need the ‘pre/postinstall’ hack (which, as you found out, doesn’t work anyway). So the problem is resolved by bumping marionette-client to 1.9.4.

> - `examples/simple` doesn't work out of the box. I had to manually symlink dependencies into place for it to work.

I created ‘postinstall’ shell script to create the necessary links.

> - The demo and the component should use `fxos-theme`, with the component using fxos-theme variables internally (see other fxos-components as example).

Fixed.

> - The pause icon is aligned a little too far to the right, resulting in a jumpy transition from play->pause.

I fixed this by adding ‘padding-left: 4px’ — taken from the video app.

> - Buttons don't have active state. I think they should be the same as the buttons in fxos-header and fxos-toolbar, which have a delayed fade-back transition on release.

From what I can tell, the buttons have an active state [1] which seems very similar to fxos-header.

[1] https://github.com/fxos-components/fxos-media-controls/blob/master/fxos-media-controls.js#L154

> - There are lots of similarities to this component and fxos-toolbar. I'm wondering if this should extend fxos-toolbar?

I had a look at fxos-toolbar.  The way I read the code, the semantics of buttons being clicked is different compared to the media controls (fxos-toolbar is opening a dialog in response to buttons being clicked; whereas the media controls are emitting an event). I'm not sure how to resolve that or whether it's beneficial to try.

> - The source contains no comments. I'm particularly interested what the 'contextmenu' event is for :) Is it 'long-press'?
Yes, it’s for long-press. I added some comments to the code.

Comment 7

[BMO Automation]

Firefox OS is not being worked on