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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
4 years ago
6 months ago

People

(Reporter: rexboy, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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
I will try to merge smart-button into gaia-button with this bug.
Assignee: nobody → im
Created attachment 8622961 [details] [review]
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.
Attachment #8622961 - Flags: feedback?(wilsonpage)
Oh! An additional item changed at this patch: I had added a test for testing keyboard support.
Hi Wilson,

You may need to use `TAB` key to move the focus in and press `ENTER` to trigger click and visual change. Thanks.
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.
Flags: needinfo?(im)
(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?
Flags: needinfo?(im) → needinfo?(wilsonpage)
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?
BTW, I had updated the code to address some suggestions/issues at comment 5, 6, 7. Please check it at the PR. Thanks.
(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 { ... }`
Flags: needinfo?(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.
(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
Flags: needinfo?(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.
Flags: needinfo?(wilsonpage)
(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.
(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.
(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?
(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?
Flags: needinfo?(tzhuang)
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.
Attachment #8622961 - Flags: feedback?(wilsonpage)
After offline discussion, John told me that we don't need build step here.
Flags: needinfo?(tzhuang)
reassign to yifan.
Assignee: im → yliao
Created attachment 8632701 [details] [review]
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!
Attachment #8632701 - Flags: feedback?(im)
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.
Attachment #8632701 - Flags: feedback?(im) → feedback+
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.
Attachment #8632701 - Flags: review?(wilsonpage)
Comment on attachment 8632701 [details] [review]
integrate keyboard behaviour into <gaia-button>

Added some questions inline :)
Attachment #8632701 - Flags: review?(wilsonpage)
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?
Flags: needinfo?(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.
Flags: needinfo?(wilsonpage)
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.
Attachment #8632701 - Flags: review?(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)
Attachment #8632701 - Flags: review?(wilsonpage)
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.
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 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.
Attachment #8632701 - Flags: review?(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?
Attachment #8632701 - Flags: review?(wilsonpage)
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.
(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?
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?

Updated

3 years ago
Blocks: 1218722

Updated

3 years ago
No longer blocks: 1218722

Updated

3 years ago
Blocks: 1219540

Updated

3 years ago
No longer blocks: 1219540

Updated

3 years ago
Assignee: yliao → nobody

Comment 35

6 months ago
Firefox OS is not being worked on
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.