refactor(select): make selection string based

[HendrikThePendric]

I've addressed the following:

• Refactored the components so that they work with string based values
• Updated docs and propTypes
• Updated demo stories
• Updated e2e stories and tests

All tests are green and when I do a full test run, I don't seem to be collecting any prop-type errors, so I think I've caught everything.

However, this PR touched a lot of files, so I won't be surprised if we end up finding some little issues.

[ismay]

Looks good, I think you got basically everything 🤘. Just a couple of very minor things:

1. We should also update the error message in MultiSelect/SelectionList.js line 20 - 22 (Github won't let me leave a comment directly for some reason).
2. Same for line 12 - 14 of SingleSelect/Selection.js.
3. And unrelated, but 119 - 123 of Select/Select.js looks redundant. Maybe that changed when implementing the popperized Menu?

edit: One thing we could do is also add some tests for what will happen with duplicate values (and other data the Select will process, but not function correctly with). So that we know how it fails, and can make sure that the failure won't change as well.

[HendrikThePendric]

@ismay Thanks for spotting those three things, I've addressed them in 1ccc223. Could you re-review?

[varl]

Now that we do not rely on the label + value, we could change the Option implementations to use the pattern we use elsewhere where the label is the children:

<Option value="smth">
  This is the label
</Option>

May be a different PR if we do that though.

[varl]

None of the commits that introduce breaking changes has a proper commit message.

[ismay]

Thanks for addressing the comments. I'm not sure about this part of the error message change:

The selected value could not be found in a child of the select..

To me that seems slightly difficult to understand. Maybe we could be more explicit, and word it as: There is no option with the value: "${value}". Make sure that all the values passed to the selected prop match an existing option.

The rest of the changes seem good to me 👍

[ismay]

And this:

One thing we could do is also add some tests for what will happen with duplicate values (and other data the Select will process, but not function correctly with). So that we know how it fails, and can make sure that the failure won't change as well.

Do you think that'd be better suited as a follow up PR, or in this one? (Or not at all is also an option of course)

[HendrikThePendric]

None of the commits that introduce breaking changes has a proper commit message.

Good catch. I'll squash my commits and add a correct footer

One thing we could do is also add some tests for what will happen with duplicate values (and other data the Select will process, but not function correctly with). So that we know how it fails, and can make sure that the failure won't change as well.

Do you think that'd be better suited as a follow up PR, or in this one? (Or not at all is also an option of course)

I'll look into this as well. It should end up being a passing test on "failing" behaviour. It'd be good to add, but I might require some assistance....

[HendrikThePendric]

Now that we do not rely on the label + value, we could change the Option implementations to use the pattern we use elsewhere where the label is the children:

<Option value="smth">
  This is the label
</Option>

May be a different PR if we do that though.

Done in 7db3e9f

[ismay]

Now that we do not rely on the label + value, we could change the Option implementations to use the pattern we use elsewhere where the label is the children:

<Option value="smth">
  This is the label
</Option>

May be a different PR if we do that though.

Done in 7db3e9f

I think that that'll break the assumption the Select wrapper makes an the options having a label prop. I believe it uses that for filtering for example.

I would retain the current option implementation. If devs need to pass something as a child they have the option to create a custom option, whilst retaining the label prop (and other assumptions the select makes about the options).

Maybe we shouldn't change too much in this PR to keep the changes manageable.

[HendrikThePendric]

I think that that'll break the assumption the Select wrapper makes an the options having a label prop. I believe it uses that for filtering for example.

Yeah, I can confirm that this is in fact happening. Somehow the test was green for me locally, but the test run here just failed due to filtering. I think filtering based on children is not really a valid approach. So I'll revert 7db3e9f.

I am still working on a test for the duplicate value behaviour though.

[HendrikThePendric]

@varl @ismay I think this PR is now ready for a final re-review:

• I've squashed my commits and included a BREAKING CHANGE section in the commit body
• I've added tests that document what happens if the selects do encounter duplicate values
• I've kept the label prop on the option components instead of switching to children, because the children approach doesn't go well with how options are being filtered.

[ismay]

Thanks for the modifications @HendrikThePendric, this looks good to merge to me (assuming that the tests will pass).

[dhis2-bot]

🎉 This PR is included in version 5.0.0-alpha.3 🎉

The release is available on:

• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dhis2-bot]

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀