1018698 - Create gaia-radio web component

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Hey Guys - anyone have time for a review or any comments? Thanks!

Comment 2

[Wilson Page [:wilsonpage]]

Comment on attachment 8432200 [details] [review]
Github pull request

kgrandon: Looks good! I'm the process of devising a component theme strategy [1]. This will require that we replace all/most PNGs with pure CSS and icon fonts. I can could take this and add the theme-ability with this patch, or add follow this up?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1019075

Comment 3

[Kevin Grandon :kgrandon]

Thanks. If that has not yet landed, I would just prefer to land close to this as possible, then take another theme pass over it.

Comment 4

[Wilson Page [:wilsonpage]]

Comment on attachment 8432200 [details] [review]
Github pull request

Comments in Github

Comment 5

[Wilson Page [:wilsonpage]]

- Need some docs/comments to explain some of the approaches, not all obvious to me :)
- CSS seems to have some redundant classes lying around

Comment 6

[Wilson Page [:wilsonpage]]

It would be super trivial to drop the PNGs with this bug too.

Comment 7

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8432200 [details] [review]
Github pull request

On the CSS side, nothing to add but Wilson's comments:
-unneeded class `pack-radio`
-we could easily remove the images and use CSS only, but that could be done in a follow up.

So after you guys agree on the JS part LGTM :)

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8432200 [details] [review]
Github pull request

Hey Wilson - did a bunch of cleanup and added some comments. Let me know what you think. Thanks!

Comment 9

[Wilson Page [:wilsonpage]]

Comment on attachment 8432200 [details] [review]
Github pull request

Good changes, although just tested the <label> content stuff and styling seems a bit broken. Some other nits in Github comments.

Perhaps something like:

<gaia-radio name="group">Label 1</gaia-radio>
<gaia-radio name="group">Label 2</gaia-radio>
<gaia-radio name="group">Label 3</gaia-radio>

Might be cleaner markup? Also wondering, are we required to provide styled labels with the component, or would the radio button alone suffice? Not sure where/if Gaia apps are using this functionality.

Comment 10

[Wilson Page [:wilsonpage]]

Attachment: Screen Shot 2014-06-05 at 11.05.07.png

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8432200 [details] [review]
Github pull request

Hey Wilson - I cleaned up the example a bit, and implemented the main example with a label I was thinking of (inside of a list).

Let me know what you think. Thanks!

Comment 12

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Kevin, I love how you've implemented the label using lists BB :)
Will be so simple to use!

Comment 13

[Wilson Page [:wilsonpage]]

I was talking to Casey about the label stuff with respect to <gaia-switch>. He said it would be nice if <label for="<radio-id>">Text</label> still worked for our form web-components. We need a consistent solution for:

- gaia-radio
- gaia-checkbox
- gaia-switch

What do you think about re-implementing <label>?

Comment 14

[Wilson Page [:wilsonpage]]

NI Casey to chime in

Comment 15

[Marco Zehe (:MarcoZ)]

Eitan did work on the Gaia Switch component's accessibility. Since label has a direct impacton that, maybe his experience can help, too. Eitan?

Comment 16

[Kevin Grandon :kgrandon]

I think we do need consistent behavior. I guess the question is should <label> live outside of the component, or from within?

I kind of feel like we will display most checkboxes, radios, and switches the same way and that having the label live inside the component is probably fine.

Comment 17

[Wilson Page [:wilsonpage]]

We could easily support both, but perhaps that is just going to complicate things

<label for="my-dangerous-radio">Text</label>
<gaia-radio id="my-dangerous-radio"></gaia-radio>

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #15)
> Eitan did work on the Gaia Switch component's accessibility. Since label has
> a direct impacton that, maybe his experience can help, too. Eitan?

Still working on these kinds of components. Bug 1017322 being the main blocker. I am really happy to see the container <label> go away here. As for the actual real label, I am not sure that our accessible naming algorithm will work for non-input elements. Need to test...

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8432200 [details] [review]
Github pull request

I'm going to go ahead and land this with Arnau's review now that we have had a few rounds of feedback. There is still an open question about how and where the <label> should be specified. I think this is something that we're going to find out through implementation, and would like to drive and structure most of the input components the same way.

After this I'd like to get the checkbox component landed (a pr is open for it), then help bring the switch component up to par.

Comment 20

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/e7debccabc206f6fcc182bd66cfea50f849339a7

Let's follow-up as necessary and iterate quickly. Thanks guys!

Comment 21

[Wilson Page [:wilsonpage]]

kgrandon: It's worth pointing out I've made significant changes to <gaia-switch> as part of bug 1019075. If changes are to be made to <gaia-switch> it might be worth pulling across the stuff I've done first. NI to get this on your radar :)

Comment 22

[Kevin Grandon :kgrandon]

Sounds good. I'll hold off on updating gaia-switch until yours is done then. Thanks!

Comment 23

[Casey Yee]

Hey guys, 
Just chiming in here in regards to the label.

My thinking is that it would would make more sense for the label to live outside of the component itself.   This would be needed to maintain the layout flexibility of the component.  There are lots of scenarios for the checkbox for instance, that would require the label text to be on the left or right of the checkbox.

Another thought here is that there was UX discussion about making the label tappable to toggle the corresponding UI control as well -- though i'm not sure that this would necessitate that the label live outside of the component necessary.