kselect refactor

[Jaspreet-singh-1032]

closes #523

• moved KeenUiSelect to KSelect and removed unused code
• renamed KeenUiSelectOption to KSelectOption

Tested it by linking kds with Kolibri
https://www.loom.com/share/66f72fc4572641a6a46c09373d319ad8?sid=6034a517-8088-4b4d-9645-8200761b5cb2

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

After review

• The changelog item has been pasted to the CHANGELOG.md

Comments

[MisRob]

Thanks @Jaspreet-singh-1032. I will review. It may take a while - lots of work here!

[MisRob]

@Jaspreet-singh-1032 The conflicts here will be caused by our rebranding work and I think it'd be simplest not to do much about it right now because there may be some further updates. I think we will just prefer updates from this PR after the review and then before merging I will make sure it's rebranded, so in this regard there won't be any action necessary from your part. Thank you for your patience.

[ozer550]

While validating one of the props value, I want to share some of the concerns:

1. Logic of computed properties are reduced which may be dangerous and may cause bugs unknowingly.
2. Inconsistent use of this.selection and this.value, even tho they are same thing we should prioritize using what was already used naming convention, if there's some specific reason we are doing this would love to know!

[Jaspreet-singh-1032]

Hi @ozer550 @MisRob, thanks for the review. I am writing down the things that need to be fixed.

• Add the help prop back
• Remove this.selection and use this.value

Let me know if you want to add anything else.

[MisRob]

Hi @Jaspreet-singh-1032, thank you. I think @ozer550 was going to do some testing in Kolibri. After that, I will give the PR the final look. I think it'd be fine to resolve @ozer550's first feedback already if you'd like to, since it seems there's agreement on it.

[MisRob]

@Jaspreet-singh-1032 We have started QA. Now it would be good to resolve the feedback from @ozer550, if you can, because after the QA, I'd like to rebase this on top of the latest work and resolve conflicts related to the rebrand.

[MisRob]

Hi @Jaspreet-singh-1032, apologies it took me such a long time and thanks one more time @ozer550 for providing first round of feedback.

I've just did my best to review as well, focusing on going through the previous KSelect API and checking that everything that needed to be moved there from KeenUiSelect is indeed there. I haven't noticed any issues, as far as I can say, and for the rest of it I think we can rely on linters and thorough QA :-)

I left one note in regards to docstrings, and I also think that multiple and multipleDelimiter logic needs to be returned as @ozer550 asked for some time ago.

---

I believe the work on this has been pretty demanding and thanks again. I learned a lot and I think the strategy for all upcoming refactors like this could be to (1) just rename and move the logic while not deleting any APIs, (2) determine and list APIs to be removed, if any (or alternatively document them if we decide to keep them), (3) do whatever was planned in the previous step gradually. If you had some reflections on how to make work and review smooth for issues like this, we'd be very glad to hear from you.

[MisRob]

@Jaspreet-singh-1032 Good news :) Our QA team hasn't noticed any problems with selects in Kolibri, except of just one that is not related to this work. Great work! I believe we'll be close to merge after the feedback I posted earlier is resolved.

[Jaspreet-singh-1032]

The multiple prop functionality is now working.

[MisRob]

Great, thanks so much, @Jaspreet-singh-1032. I will take it from here and resolve the conflict related to the rebranding because I think it's best to have some more background for that.

[MisRob]

I resolved conflicts in style and tested one more time on the documentation page as well as at a few places in Kolibri. All looking good to me, thanks everyone.

[Jaspreet-singh-1032]

Thanks @MisRob