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

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: danhuang, Assigned: danhuang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Move smart-icons to fxos-tv-icons
(Assignee)

Updated

3 years ago
Blocks: 1230876
(Assignee)

Updated

3 years ago
Assignee: nobody → dhuang
(Assignee)

Updated

3 years ago
Summary: [TV] move smart-icons to fxos-tv-icons → [TV] Move smart-icons to fxos-components and rename it to fxos-tv-icons
(Assignee)

Updated

3 years ago
Priority: -- → P2
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8704927 [details] [review]
patch for moving smart-icons into fxos-components
(Assignee)

Comment 2

3 years ago
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.
Attachment #8704927 - Flags: review?(rexboy)
(Assignee)

Comment 3

3 years ago
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.
Attachment #8704927 - Flags: feedback?(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 ;)
Attachment #8704927 - Flags: feedback?(wilsonpage) → feedback+
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?
Attachment #8704927 - Flags: review?(rexboy) → review+
(Assignee)

Updated

3 years ago
Blocks: 1238868
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

3 years ago
Merge in master: 
https://github.com/fxos-components/fxos-tv-icons/commit/714bd416967da7a494c185613049e15edd8182ef
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
No longer blocks: 1238868
You need to log in before you can comment on or make changes to this bug.