fxos-components / fxos-switch

0 stars 5 forks source link

Change gaia-switch to accept a label as a slot #13

Open KevinGrandon opened 9 years ago

KevinGrandon commented 9 years ago

Currently you can't click on labels to toggle the switch, and you should be able to do so.

The current gaia-switch component in gaia shared/ passes in the label to the component, and is a nicer way of going about things I think: https://github.com/mozilla-b2g/gaia/blob/540701791684cc527dccc9b7f1d4b694c7ce296f/apps/ftu/index.html#L262

This also works in pretty much every use case that we have for switches in gaia.

wilsonpage commented 9 years ago

We do support the use of labels with a for attribute.

I'm not fan of nesting <label> inside <gaia-switch>. If we want implicit <label> -> <gaia-switch> click retargeting I think this issue is best solved properly with a <gaia-label> component that knows about which nested components (eg. <gaia-switch>, <gaia-checkbox>, etc) need special behaviour.

KevinGrandon commented 9 years ago

I'll leave it up to you. Nesting labels inside of gaia-switch controls will make the transition between the existing gaia-switch in gaia and this component extremely easy.

KevinGrandon commented 9 years ago

After working with many of the legacy components in gaia over the last few weeks, I'd just like to leave my experiences here.

The legacy components in gaia/shared use a nested label inside each component for the ease of porting them. It's consistent with building blocks (not really a good reason), but also many apps have custom styling the a single building block (the switch + label). I think breaking the switch component out into two pieces is going to cause a lot of extra migration work, and break things.

Do you think it would be possible to have support for both nested labels, and external labels? This would make porting the existing usage of web components and building blocks extremely simple.

wilsonpage commented 9 years ago

Does the styling of these nested labels have to be included here?

KevinGrandon commented 9 years ago

Does the styling of these nested labels have to be included here?

I imagine it would, but maybe you could do something tricky. Alternatively we could have another parent class, something like "ComboSwitch", but this does feel a little too verbose to me.

<switch-control>
  <gaia-label />
  <gaia-switch />
</switch-control>
wilsonpage commented 9 years ago

IMO it would be simpler to just create a simple <gaia-label> and replace existing CSS building-blocks with:

<gaia-label>Title<gaia-switch/></gaia-label>

This way we define the label styling in one place to be used alongside <gaia-switch>, <gaia-checkbox> and <gaia-radio>, instead of duplicating label styles in all three components.

This approach is extensible, it reflects how built-in form controls work today and means we won't be living with the nested label hack for x years.

If you don't want the hassle of doing this, we have the for attribute which is usable today.

KevinGrandon commented 8 years ago

@wilsonpage - In order to easily port gaia-components I think we need a drop-in replacement for the existing gaia-switch (and other components), being used. It could either be this component, or potentially a new component. Here's a few ideas:

Option 1: Make fxos-switch accept a label, like:

<fxos-switch>
  <label>My Label</label>
</fxos-switch>

Option 2: Create a new "fxos-switch-row". This could be an external component.

<fxos-switch-row>
  <label>My Label</label>
  <details>Some details here</details>
</fxos-switch-row>

Option 3: Continue to use inside of gaia, and update it to extend or include the . This could work, but I do think that we should only have one switch implementation inside of gaia, and currently almost every switch spans across the entire row.

Re-opening this to get ideas, but maybe the work will happen in a different repo. Thoughts?

KevinGrandon commented 8 years ago

After porting most switches across gaia to web components, I think it's way too much work to move away from the nested label control at this time. I think our time is better spent implementing additional web components.