WIP: SelectableTileGroup

[ispyinternet]

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-svelte/carbon-components-svelte/plux0963q
✅ Preview: https://carbon-components-svelte-git-master.carbon-svelte.vercel.app

[metonym]

In addition to passing the CI/deploy preview, could you remove the committed package-lock.json files?

[ispyinternet]

In addition to passing the CI/deploy preview, could you remove the committed package-lock.json files?

Should I update .gitignore?

I also inadvertantly added palimpsest, should I add this to gitignore?

[ispyinternet]

BTW, you said in the issue comments to use selectedItem selectedItems which also is breaking? Wasn't sure if you realised?

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

SelectableTileGroup – bind:selectedValues RadioTileGroup – bind:selectedValue

[metonym]

How about creating separate PRs that address the issues individually?

• DataTableSkeleton #423
• new SelectableTileGroup component #428
• new RadioTileGroup component

[metonym]

BTW, you said in the issue comments to use selectedItem selectedItems which also is breaking? Wasn't sure if you realised?

selectedValue and selectedValues would be new props if SelectableTileGroup and RadioTileGroup components are introduced.

[ispyinternet]

Wow, that was painful. Mainly due to my own ineptitude, but having to push to debug. Can you advise on a setup to test the components locally.

I'm sorry If you've been getting pinged senseless over these commits!

1 or 2 unanswered above and advise me of any other changes you require.

[metonym]

For testing locally, you can use the doc website as a UI.

Refer to CONTRIBUTING.md: https://github.com/IBM/carbon-components-svelte/blob/master/CONTRIBUTING.md#documentation-set-up

[ispyinternet]

• remove any changes to TileGroup/RadioTile
• remove RadioTileGroup
• restore the "light" variant example for SelectableTile

You did originally ask to bring in the RadioTileGroup? I know i've made this painful - but do I need to strip out RadioTileGroup?

[ispyinternet]

• remove any changes to TileGroup/RadioTile
• remove RadioTileGroup
• restore the "light" variant example for SelectableTile

I've restored the light, I think the reason I stripped it because when I reviewed the live docs, it wasn't actually being demod properly:

https://carbon-svelte.vercel.app/components/RadioTile

[ispyinternet]

i haven't removed RadioTileGroup yet - I'm fighting to keep it - everything is still backwards compatible?

[metonym]

i haven't removed RadioTileGroup yet - I'm fighting to keep it - everything is still backwards compatible?

Save it for another PR

[ispyinternet]

i haven't removed RadioTileGroup yet - I'm fighting to keep it - everything is still backwards compatible?

Save it for another PR

grunting to self.

[metonym]

Are you able to get two way binding on selectedValues to work?

<script>
  let selectedValues = ["1"]
</script>

<SelectableTileGroup bind:selectedValues legend="Select the options you require">
  <SelectableTile title="Option 1" value="1">
    Option 1
  </SelectableTile>
  <SelectableTile title="Option 2" value="2">
    Option 2
  </SelectableTile>
  <SelectableTile title="Option 3" value="3">
    Option 3
  </SelectableTile>
</SelectableTileGroup>

[metonym]

@ispyinternet take a look at this: https://svelte.dev/repl/bf4a1ebbe12b49d38f0fcd15ca782a04?version=3.30.1

It supports three use cases using component composition:

1) Binding w/ a single prop

The initial selected items are in sel.

<script>
  let sel = ['0']
</script>

<Parent bind:sel>
  <Child id="0" />
  <Child id="1" />
  <Child id="2" />
</Parent>

2) Binding w/ a single prop (initially selected children)

The initial selected items are set using the selected prop on a child component.

<script>
  let sel = [];

  $: console.log(sel); // --> ['0']
</script>

<Parent bind:sel>
  <Child id="0" selected />
  <Child id="1" />
  <Child id="2" />
</Parent>

3) "controlled"

<script>
  let controlled = [
    { id: "0", selected: true },
    { id: "1", selected: false },
    { id: "2", selected: false },
  ];
</script>

<Parent sel={controlled.filter(i => i.selected).map(i => i.id)}>
  {#each controlled as item}
    <Child id="{item.id}" bind:selected="{item.selected}" />
  {/each}
</Parent>

[ispyinternet]

@ispyinternet take a look at this: https://svelte.dev/repl/bf4a1ebbe12b49d38f0fcd15ca782a04?version=3.30.1

It supports three use cases using component composition:

1) Binding w/ a single prop

<script>

  let sel = ['0']

</script>



<Parent bind:sel>

  <Child id="0" />

  <Child id="1" />

  <Child id="2" />

</Parent>

2) Binding w/ a single prop (initially selected children)

<script>

  let sel = [];

</script>



<Parent bind:sel>

  <Child id="0" selected />

  <Child id="1" />

  <Child id="2" />

</Parent>

3) "controlled"

<script>

  let controlled = [

    { id: "0", selected: true },

    { id: "1", selected: false },

    { id: "2", selected: false },

  ];

</script>



<Parent sel={controlled.filter(i => i.selected).map(i => i.id)}>

  {#each controlled as item}

    <Child id="{item.id}" bind:selected="{item.selected}" />

  {/each}

</Parent>

Yh, I was being a complete donut I was binding to the wrong variable name 😩

[ispyinternet]

So I've fixed the binding bug, since this is a new component, do you want to include support for light at the group level?

[ispyinternet]

Hi @metonym did you get a chance to review this and do you want me me to look at anything? Cheers.

[metonym]

@ispyinternet Does it fulfill the three test cases mentioned above?

1. bind w/ a single prop
2. bind w/ a single prop with initially selected children
3. "controlled"

[ispyinternet]

I'm pretty sure it does, but I assumed that you would want to validate that yourself before commiting to master.

I had asked myself if there were any way to write tests against this? I don't think the current project supports this does it?

If you want me to prepare a demo to validate the three cases I would be happy to (in the coming days)

[metonym]

@ispyinternet I added an example that tests the three use cases: https://carbon-components-svelte-onddzi298.vercel.app/framed/SelectableTile/SelectableTileReactive

Click around in the first two. The bound selectedValues is indeed updating correctly by clicking tiles. However, I'm unable to programmatically update the selectedValues by clicking "reset selectedValues."

The issue here is that SelectableTileGroup has to update the internal selected values store based on the exported selectedValues prop. This is illustrated in the REPL link that I'd shared.

In the third use case ("controlled"), I am able to reset the selectedValues, but when I click on the one of the tiles the selectedValues is incorrectly updated.

[ispyinternet]

Ok ill get this sorted.

[ispyinternet]

In the third use case ("controlled"), I am able to reset the selectedValues, but when I click on the one of the tiles the selectedValues is incorrectly updated.

@metonym what I'm struggling with atm, is the additional (guessable) api of the selectedValues prop.

That is, we can either supply an array of scalars, or an array of { value, selected} objects.

In the later case, suppose we empty the array, (unselect all), then select, we will need to know what the user expects back - an array of scalar values or an array of object types. We would have to infer by the initial incoming state and potentially 'remember'. There is of course the potential then to allow users to provide:

let selectedValues = [
1, // assume { value: 1, selected: true }
2,
{ value: 'a', selected: true }
]

I guess if we detect any item of the 'object form' in the initial creation, then always return that form?

[metonym]

@ispyinternet Perhaps I was unclear. Take the Svelte REPL as a direct reference.

That is, we can either supply an array of scalars, or an array of { value, selected} objects.

Not sure what you mean by this.

selectedValues should be string[]. It shouldn't be an array of objects. See the Svelte REPL.

[ispyinternet]

@ispyinternet Perhaps I was unclear. Take the Svelte REPL as a direct reference.

That is, we can either supply an array of scalars, or an array of { value, selected} objects.

Not sure what you mean by this.

selectedValues should be string[]. It shouldn't be an array of objects. See the Svelte REPL.

Ok (array any!) however your framed example did bind to selectedValues3 which is the array of objects!

EDIT Well it didn't bind but it did use initial value. Do you want to support that?

[ispyinternet]

@metonym I have to admit I'm stumped on the last example. I can't get an atomic update of the store. Basically, I'm reasoning that as the control updates the each loop, when the first item updates, the store updates, which passes an update back down to the children - it will only reset one item at a time. I attempted to use #key but that doesn't fix it:

https://carbon-components-svelte-plux0963q.vercel.app/framed/SelectableTile/SelectableTileReactive

[ibm-ci-bot]

@ispyinternet: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ibm-ci-bot]

@ispyinternet: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ibm-ci-bot]

@ispyinternet: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ibm-ci-bot]

@ispyinternet: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.