carbon-design-system / carbon-components-svelte

Svelte implementation of the Carbon Design System
https://svelte.carbondesignsystem.com
Apache License 2.0
2.71k stars 261 forks source link

RadioTile support 'multiple' prop? #428

Open ispyinternet opened 4 years ago

ispyinternet commented 4 years ago

Initially I was looking at SelectableTile to control a selection of filters, firstly with there being nothing reactive about it, it looked like a bit of clunky end user wiring would be required - that's another issue I wonder should be looked at?

Anyway for my use case, I was happy to allow only 1 filter, so I opted for the much more succint RadioTile, but it did get me thinking if I wanted to support more than one selection, should this component support a multiple prop?

If you did want to implement, how would you like to handle the value? Array type when multiple, or Array type always?

metonym commented 4 years ago

...should this component support a multiple prop?

Radio buttons must only have one selection (Nielsen Norman).

I'll review the SelectableTile.

ispyinternet commented 4 years ago

I guess I've also just discovered that at least one must be selected as I look for a way to empty the filters!

Yes I think if SelectableTile could fill the gap that would be great. If you want to suggest a solution I can look at it. Would an optional wrapper component be a feesible solution?

metonym commented 4 years ago

Sure – that approach is sound.

Similar to RadioTileGroup, the wrapper component (e.g., SelectableTileGroup) would manage the selected ids using the context API.

ispyinternet commented 4 years ago
  1. Do you want to rename TileGroup -> RadioTileGroup ?
  2. Or what do you think about keeping TileGroup and using a prop to specify radio or select? that would then make RadioTile and SelectableTile one in the same -> Tile with selectable prop?
metonym commented 4 years ago

I like your thinking of reusing TileGroup.

One issue though: the selected prop type would need to change to be an array of values. So for a RadioTile you would need to do selected[0].

I'm leaning towards keeping them separate. Preserve the TileGroup component but add two new ones dedicated to the SelectableTile, RadioTile respectively:

So we can deprecate TileGroup in the next major release in one fell swoop.

ispyinternet commented 4 years ago

At the moment, I guess these tile groups are suffering the same fate as the Tabs in that they are not programmatically mutable?

Might be good to try and address this with the newer versions? Although I wasn't keen on the sight of the all the if statements! Are there no other options / techniques on the table?

EDIT: I guess this isn't an issue since order isn't important?

ispyinternet commented 4 years ago

@metonym as per original discussion I've written the code to make the wrapping SelectableTileGroup optional, but I'm now wondierng if this is really necessary? Should we just say SelectableTile's have to be composed as children of SelectableTileGroup as do RadioTile's?

see: https://gist.github.com/ispyinternet/862b6f26c0cb461e132f28062ce1f046

metonym commented 4 years ago

That's a breaking change but I think it's necessary to fully support reactive/programmatic control.

metonym commented 4 years ago

I guess this isn't an issue since order isn't important?

I don't think we need to worry about this. The key API would be the escape hatch.

ispyinternet commented 4 years ago

the linked code is non breaking, but when coming to write the docs, I'm wondering what is the point in showing one group with by a div (the non breaking/ current usage) and another group wrapped by the SelectableTileGroup? If we say yes go with breaking change I can remove the relevant checks on the context.

metonym commented 4 years ago

Yes – update the docs to remove the div usage and replace it with the SelectableTileGroup