1236823 - [TV] Move smart-icons to fxos-components and rename it to fxos-tv-icons

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV, defect, P2)

Description

[Dan Huang[:danhuang]]

Move smart-icons to fxos-tv-icons

Comment 1

[Dan Huang[:danhuang]]

Attachment: patch for moving smart-icons into fxos-components

Comment 2

[Dan Huang[:danhuang]]

Comment on attachment 8704927 [details] [review]
patch for moving smart-icons into fxos-components

Hi rex, please help review this patch, this patch rename 'smart-icons' to 'fxos-tv-icons', and also update package.json for npm publish. Thanks.

Comment 3

[Dan Huang[:danhuang]]

Comment on attachment 8704927 [details] [review]
patch for moving smart-icons into fxos-components

Hi Wilson, please help to check this patch. This patch refer 'fxos-icons'. The update in this patch is: 1. rename 'smart-icons' to 'fxos-tv-icons', 2. update package.json for npm publish. Thanks.

Comment 4

[Wilson Page [:wilsonpage]]

Comment on attachment 8704927 [details] [review]
patch for moving smart-icons into fxos-components

Patch LGTM. Thoughts:

- I'm surprised TV needs it's own icon set. I'd like to see the two merge eventually.

- For fxos-icons we use Docker to ensure consistent build env. We found building on Mac lead to some strange perf issues on smartphone. Not sure why. Don't worry about this it you aren't seeing this on TV. Or just make sure to always build on a Linux box :)

Thanks y'all ;)

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8704927 [details] [review]
patch for moving smart-icons into fxos-components

Looks good to me, but I don't quite understand how to include files that are in  node_components from an app. Is there any build scripts for this under going?

Comment 6

[Dan Huang[:danhuang]]

(In reply to Wilson Page [:wilsonpage] from comment #4)
> Comment on attachment 8704927 [details] [review]
> patch for moving smart-icons into fxos-components
> 
> Patch LGTM. Thoughts:
> 
> - I'm surprised TV needs it's own icon set. I'd like to see the two merge
> eventually.
> 
> - For fxos-icons we use Docker to ensure consistent build env. We found
> building on Mac lead to some strange perf issues on smartphone. Not sure
> why. Don't worry about this it you aren't seeing this on TV. Or just make
> sure to always build on a Linux box :)
> 
> Thanks y'all ;)

Thanks for your feedback, I also think it would be better to merge two icon set together. And the build issue seems didn't happen on TV.

Comment 7

[Dan Huang[:danhuang]]

(In reply to KM Lee [:rexboy] from comment #5)
> Comment on attachment 8704927 [details] [review]
> patch for moving smart-icons into fxos-components
> 
> Looks good to me, but I don't quite understand how to include files that are
> in  node_components from an app. Is there any build scripts for this under
> going?

Thanks for the review. I think the usage should be: 1. add fxos-tv-icon dependency in package.json 2. run 'npm install' in apps 3. index.html reference to node_modulse/fxos-tv-icons/fxos-tv-icons.css

The follow bug: 1238868 would be use to replace smart-icons to fxos-tv-icons in tv_apps.

Comment 8

[Dan Huang[:danhuang]]

Merge in master: 
https://github.com/fxos-components/fxos-tv-icons/commit/714bd416967da7a494c185613049e15edd8182ef