1144540 - [Stingray][Component] Unify package format for smart-button with other gaia-components

Status: RESOLVED WONTFIX

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

Description

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

gaia-components/smart-button currently keeps its original format when it was in smart-components due to historical reasons. We would try to unify it with other gaia-components. Namely:

1. Use unified prefix for tag name. (Still need discuss, in bug 1140973)
2. Use my-component-name.js rather than script.js and style.css
3. Use gaia-components base class (Still need discuss, in bug 1140973)
4. Use UMD wrapper
5. Use Karma and write unit tests.
6. move example page to /index.html
7. Create gh-pages branch for live demo

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

I will try to merge smart-button into gaia-button with this bug.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: merge smart button to gaia button

Hi Wilson,

This is the early patch for the merge of smart button and gaia button. I am totally new to the new arch of gaia components. We use old style to create smart button. So, I hope you can see this patch before I continue to move on.

We don't have a list of shared variable for theme in TV now. Unlike phone, I just use the value in this patch. Maybe, UX and VX may give us more information on theme in the future.

This is the first button visual. We have 7 button visuals in total. So, the style may be so long after finished. Any suggestions for this one?

This patch contains:
1. keyboard support (only enter/ key)
2. add base button style for tv
3. add toggled state for tv. We need this state to display button as toggled mode.
4. use deviceType to separate the UI from tv and phone

If you feel the naming is inaccurate, please tell me. I would love to fix them.

BTW, we may discuss this at work week, too.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Oh! An additional item changed at this patch: I had added a test for testing keyboard support.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi Wilson,

You may need to use `TAB` key to move the focus in and press `ENTER` to trigger click and visual change. Thanks.

Comment 5

[Wilson Page [:wilsonpage]]

Can you tell me what the addition features/requirements are of the tv gaia-button compared to normal gaia-button?

It would be nice if we could remove the word 'tv' altogether and just explain the characteristics of the button; for example 'large' mode.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Wilson Page [:wilsonpage] from comment #5)
> Can you tell me what the addition features/requirements are of the tv
> gaia-button compared to normal gaia-button?

We don't have any additional features or requirements for tv gaia-button. We just have different visuals for it. But for app implementation, we may need to have few properties to draw button as focused, toggled, etc. 

The visual spec can be found at https://drive.google.com/a/mozilla.com/folderview?id=0B2-G3kew1WpXMjBQSTFCay1TalE&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk


> It would be nice if we could remove the word 'tv' altogether and just
> explain the characteristics of the button; for example 'large' mode.

Thanks for this suggestion. May I think that I should replace the "deviceType" property with "large"? In smart-button, we use "type" property to differentiate button types. We have so 8 button types in total. May user be confused to have different properties?


BTW, I had rethink the huge CSS string yesterday. How about we use gulp to compile and merge CSS and JS?

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

I got another issue when I tried to use `i` to host data icon inside of gaia-button, just like gaia-button.

The CSS rule with `gaia-button[type='large'] .content i:before` doesn't work. I cannot make sure if it should be shadow DOM issue. So, the only way to change the style of icon is to use `gaia-button[type='large'] i:before`. Am I correct?

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

BTW, I had updated the code to address some suggestions/issues at comment 5, 6, 7. Please check it at the PR. Thanks.

Comment 9

[Wilson Page [:wilsonpage]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #7)
> I got another issue when I tried to use `i` to host data icon inside of
> gaia-button, just like gaia-button.
> 
> The CSS rule with `gaia-button[type='large'] .content i:before` doesn't
> work. I cannot make sure if it should be shadow DOM issue. So, the only way
> to change the style of icon is to use `gaia-button[type='large'] i:before`.
> Am I correct?

Try: `:host([type='large']) ::content i:before { ... }`

Comment 10

[Wilson Page [:wilsonpage]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #6)
> (In reply to Wilson Page [:wilsonpage] from comment #5)
> > Can you tell me what the addition features/requirements are of the tv
> > gaia-button compared to normal gaia-button?
> 
> We don't have any additional features or requirements for tv gaia-button. We
> just have different visuals for it. But for app implementation, we may need
> to have few properties to draw button as focused, toggled, etc. 

These sound like useful addition for all button types, not just tv :)

> The visual spec can be found at
> https://drive.google.com/a/mozilla.com/folderview?id=0B2-
> G3kew1WpXMjBQSTFCay1TalE&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk

A (very) quick scan of the spec seems that TV buttons are larger and also have a 'progress' mode.

> 
> > It would be nice if we could remove the word 'tv' altogether and just
> > explain the characteristics of the button; for example 'large' mode.
> 
> Thanks for this suggestion. May I think that I should replace the
> "deviceType" property with "large"? In smart-button, we use "type" property
> to differentiate button types. We have so 8 button types in total. May user
> be confused to have different properties?

What are the 8 types we need to support?

> 
> BTW, I had rethink the huge CSS string yesterday. How about we use gulp to
> compile and merge CSS and JS?

I'd rather avoid a build step unless it's absolutely required.

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Wilson Page [:wilsonpage] from comment #10)
> A (very) quick scan of the spec seems that TV buttons are larger and also
> have a 'progress' mode.

Well, this is the complex one. We didn't implement it, yet. We only implemented the one we already used in tv_apps.

> What are the 8 types we need to support?

1. Circle button ( Icon only ) => spec 1.1
2. Circle button ( Text only ) => spec 1.3
3. Button ( Link ) => spec 1.5
4. Button ( Text only ) => spec 1.6
5. Button ( Icon with Text ) => spec 1.7
6. App Button in App Deck => app deck spec 1.4 [1]
7. App Button in Home => home app spec 3.1 [2]
8. Banner Button => app deck spec 1.3 [1]

We use CSS override to shrink the button size at item 6 base on item 7. But I will create one for it.

> I'd rather avoid a build step unless it's absolutely required.
Ok. We have 400+ lines of style. If I put all of them to gaia-button.js, this file may be near 900 lines. Hope that it is fine to us.


[0] Building block spec: https://drive.google.com/a/mozilla.com/folderview?id=0B2-G3kew1WpXMjBQSTFCay1TalE&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk
[1] App deck spec:  https://drive.google.com/a/mozilla.com/folderview?id=0B2-G3kew1WpXajJRV3hkeUw4QjA&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk
[2] Smart home spec: https://drive.google.com/a/mozilla.com/folderview?id=0B2-G3kew1WpXdHNCUXN4Q2lMbnc&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk
[3] Animation spec: https://drive.google.com/a/mozilla.com/folderview?id=0B2-G3kew1WpXTzg0TXR4M2xWb1U&usp=sharing&tid=0B2-G3kew1WpXLURNc29vTWl5eUk

Comment 12

[Wilson Page [:wilsonpage]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #11)
> > I'd rather avoid a build step unless it's absolutely required.
> Ok. We have 400+ lines of style. If I put all of them to gaia-button.js,
> this file may be near 900 lines. Hope that it is fine to us.

Hmmmm didn't realise it was so large. Perhaps we should extend <gaia-button> then to create <gaia-tv-button> or something? Seems like a lot of code weight to parse/carry for apps that aren't going to be used on TV?

If you want to go down this route and can explain how to extend a component.

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Wilson Page [:wilsonpage] from comment #12)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #11)
> > > I'd rather avoid a build step unless it's absolutely required.
> > Ok. We have 400+ lines of style. If I put all of them to gaia-button.js,
> > this file may be near 900 lines. Hope that it is fine to us.
> 
> Hmmmm didn't realise it was so large. Perhaps we should extend <gaia-button>
> then to create <gaia-tv-button> or something? Seems like a lot of code
> weight to parse/carry for apps that aren't going to be used on TV?
> 
> If you want to go down this route and can explain how to extend a component.

Sorry, I don't get your point from comment 11.

This patch only has one type of button. If I merge 8 types of button to gaia-button, I will have 400+ lines of CSS, only CSS.

If we go back to gaia-tv-button, should we just rename smart-button to gaia-tv-button and keep them as separate.

I prefer to merge the to gaia-button personally. That's the reason I propose to have a build procedure at comment 6.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Wilson Page [:wilsonpage] from comment #9)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #7)
> > I got another issue when I tried to use `i` to host data icon inside of
> > gaia-button, just like gaia-button.
> > 
> > The CSS rule with `gaia-button[type='large'] .content i:before` doesn't
> > work. I cannot make sure if it should be shadow DOM issue. So, the only way
> > to change the style of icon is to use `gaia-button[type='large'] i:before`.
> > Am I correct?
> 
> Try: `:host([type='large']) ::content i:before { ... }`

Thanks for this one. It works well.

Comment 15

[Wilson Page [:wilsonpage]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #13)
This patch only has one type of button. If I merge 8 types of button to
> gaia-button, I will have 400+ lines of CSS, only CSS.
> 
> If we go back to gaia-tv-button, should we just rename smart-button to
> gaia-tv-button and keep them as separate.
> 
> I prefer to merge the to gaia-button personally. That's the reason I propose
> to have a build procedure at comment 6.

What exactly would the build step do?

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Wilson Page [:wilsonpage] from comment #15)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #13)
> > I prefer to merge the to gaia-button personally. That's the reason I propose
> > to have a build procedure at comment 6.
> 
> What exactly would the build step do?

Basically, I am not an expert of gulp. Someone in our team is. Let us ask him.

IMHO, we can merge all CSS files into a field named `style` in gaia-button. There may be file inclusion and resource path issue. We should also resolve the inclusion and patch through gulp.

dwi2, may you give us some suggestions of gulp?

Comment 17

[Wilson Page [:wilsonpage]]

After my chat with :johnhu I think agreed on the following:

- Extract smart keyboard behaviour from <smart-button> (perhaps in a library) and integrate into into core <gaia-button>

- Extend <gaia-button> to create <gaia-tv-button> which includes alternative styling. I can help get this started, it may need some tweaks to gaia-component.js base class.

Comment 18

[Tzu-Lin Huang [:dwi2][:tzhuang]]

After offline discussion, John told me that we don't need build step here.

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

reassign to yifan.

Comment 20

[yifan [:yifan][:yliao]]

Attachment: integrate keyboard behaviour into <gaia-button>

Removed tv-button style & js from John's patch. @john could you please see if this is the correct way to integrate keyboard behaviour? Thank you!

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Everything looks good. We may need a button to display keyboard support instead of TV buttons at this PR.

Comment 22

[yifan [:yifan][:yliao]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Hi Wilson, could you please review this patch? Thank you!

It adds keyboard behavior according to the first item in comment 17.

Comment 23

[Wilson Page [:wilsonpage]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Added some questions inline :)

Comment 24

[yifan [:yifan][:yliao]]

Thanks! While I'm working on the questions, I'd like to confirm that should we 1) merge keyboard behaviors into gaia-button directly or 2) write it in another js file and require it?

Comment 25

[Wilson Page [:wilsonpage]]

(In reply to yifan [:yifan][:yliao] from comment #24)
> Thanks! While I'm working on the questions, I'd like to confirm that should
> we 1) merge keyboard behaviors into gaia-button directly or 2) write it in
> another js file and require it?

Let's keep it inside this this component for the time being an look to extract if/when we need it elsewhere.

Comment 26

[yifan [:yifan][:yliao]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Thank you John & Wilson for the review! I've updated the PR according to comments.

Comment 27

[Wilson Page [:wilsonpage]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Thanks for the updates :)

I checked out the patch locally and tried using the keyboard:

- I saw no visual change when buttons were focused
- I saw no visual change when the enter key was pressed whilst a button was focused.

What is the expected outcome of this patch?

(Also some other comments on Github)

Comment 28

[John Hu [:johnhu][:johu][:醬糊小弟]]

We don't have visual for focused and key pressed now. So, this patch is only for add the code to support it. And we will use them at tv button which extends gaia-button.

Comment 29

[John Hu [:johnhu][:johu][:醬糊小弟]]

We don't have visual for focused and key pressed now. So, this patch is only for add the code to support it. And we will use them at tv button which extends gaia-button.

Comment 30

[yifan [:yifan][:yliao]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Thank you for the review! Revised according to the nits.

* Fix typo.
* Extract out the 'toggled' attribute test.
* Remove the 'released' attribute.

Comment 31

[Wilson Page [:wilsonpage]]

Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

I'm still a little confused by the 'toggled' feature. I'm guessing that it is used in TV apps to toggle a button in a similar way to how a checkbox/switch is toggled?

Should the visual state of a 'toggled' button be identical to the 'pressed'/`:active` state, or is it different?

In the core gaia-button I'd like to see the visual 'keydown' state of the button be the same as `:active`. Does this match the spec for tv button?

Comment 32

[yifan [:yifan][:yliao]]

An example of 'toggled' is the text buttons used on the tv homescreen. The difference between 'toggled' and :active is that :active only applied between mouse keydown and keyup. In short yes 'toggled' is used kind of like a checkbox/switch.

Comment 33

[Wilson Page [:wilsonpage]]

(In reply to yifan [:yifan][:yliao] from comment #32)
> An example of 'toggled' is the text buttons used on the tv homescreen. The
> difference between 'toggled' and :active is that :active only applied
> between mouse keydown and keyup. In short yes 'toggled' is used kind of like
> a checkbox/switch.

Is the visual state of :active and 'toggled' the same?

Comment 34

[yifan [:yifan][:yliao]]

No, on the tv homescreen. I probably understand the concern here... it can be removed and be added to the later inherited tv button only. Is this OK?

Comment 35

[BMO Automation]

Firefox OS is not being worked on