Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue

[muditchoudhary]

Description

• This PR, creates a new KRadioButtonGroup component that fixes radio button movement with arrow keys issues in Firefox.
• It also has some Unit tests for KRadioButtonGroup.
• It creates docs for KRadioButtonGroup

Issue addressed

Fixes: 10491

Addresses #PR# HERE

Before/after screenshots

Before

Before.mp4

After

After.mp4

Changelog

• [#PR no]
  • Description: Summary of change(s)
  • Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
  • Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
  • Components: Affected public KDS component. Do not include internal sub-components or documentation components.
  • Breaking: Will this change break something in a consumer? Choose from: yes / no
  • Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
  • Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

[#PR no]: PR link

Steps to test

1. Yarn link the Kolibri with KDS. How? given: here
2. Import KRadioButtonGroup component and wrap KRadioButton components inside KRadioButtonGroup component, in these files:
   2.1 kolibri/plugins/device/assets/src/views/DeviceSettingsPage/index.vue
   2.2 kolibri/core/assets/src/views/language-switcher/LanguageSwitcherModal.vue
3. Run Server and test in Firefox by visiting these pages:
   3.1 http://127.0.0.1:8000/en/device/#/settings
   3.2 Choose Language

Second way

1. Paste the given playground.vue code. here
2. Run the KDS server and visit playground and test.

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

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

For the change language model KGrid is used, which creates two KRadioButtonGroup components as the array is split.
Although the arrow keys are working fine but when we shift tab it puts focus on the last focus radio button on the first list.
I want to know your thoughts. Is that acceptable?

[muditchoudhary]

Hi @MisRob Could you please review the PR?

[MisRob]

Hi @muditchoudhary, thanks a lot! I think as first step, we will QA in Kolibri. Would you be able to open a Kolibri PR for testing that would reference the latest commit of this KDS PR and use KRadioButton at least at one Kolibri place? Our QA team doesn't work with code directly. If you couldn't do that, I can prepare the PR as well.

[muditchoudhary]

Sure @MisRob I had tested on Kolibri and I have the changes in stash, so I will have no problem in making a PR. I will send a PR soon.

[MisRob]

Thanks so much @muditchoudhary, firstly for researching the issue and approaches to solve it, and then in this PR for finalizing it. Also thanks for taking care of docs and tests. Overall it's great work and feels solid.

I'm leaving a couple of comments. The most important is the one related to many event listeners and it's the only change I request before proceeding to merging. I'd also like to clarify the enable logic.

I'd appreciate if we could have the improvement regarding the dependency to KGridItem, however it's also something that I could open a follow-up issue for. Same applies to the remaining comments that are rather minor. It really depends on your bandwith - let me know what works for you.

[muditchoudhary]

Thanks a lot, @MisRob for giving the detailed review and suggesting changes. I will send the changes soon.

[muditchoudhary]

Hi @MisRob I have made some changes in the code as per your suggestions [NOT PUSHED THE COMMITS YET] . All the things are good except with input listener for which I have asked you for some help here.

And regarding the enable logic I have answered here

[MisRob]

Thanks a lot @muditchoudhary! Just answered. Seems we're close, great work.

[muditchoudhary]

Hi @MisRob I have pushed the commit. Could you please check?

Also, please confirm I need to target the develop branch, right?

Thank you!!

[muditchoudhary]

There were some much mege conflicts with KCard too. I resolved it

[MisRob]

Thanks @muditchoudhary! Yes, develop, that's right. Would you mind also updating learningequality/kolibri#12325 to point to this latest work, so that we can QA again in Kolibri? There were lots of changes, so it'd be great to re-test.

[MisRob]

Would you mind also updating learningequality/kolibri#12325 to point to this latest work, so that we can QA again in Kolibri? There were lots of changes, so it'd be great to re-test.

@muditchoudhary I have some time so I will do this now

[MisRob]

@muditchoudhary Thanks again, Mudit. This is fantastic progress and I think we're close to merge. Ccan't wait to see it live (as a Firefox user myself :)

The only important thing to resolve is the recursion bug. Besides I left a couple of minor non-blocking notes that are optional.

[muditchoudhary]

@MisRob Thank you!! so much ❤️ for reviewing the code and helping me with the recursive logic.
I have updated the recursive logic and methods name as you have suggested. I updated the KDS fork version too with latest commit for QA.

Once the QA successfully tests the PR, I will rebase this branch with develop for merging.

Edit:
Here is the playground.vue code which I have used for testing KRadioButtonGroup with new recursive logic
https://gist.github.com/muditchoudhary/66f1663e457d5dd455d5190b7058ccd5

[MisRob]

Great @muditchoudhary, thanks a lot too! Joy on my side. I will have a look at the latest changes, hopefully some time this week, and also ask my colleagues for final QA run on Kolibri. And thank you for sharing the playground code, that's helpful.

[MisRob]

@muditchoudhary All new code looks well to me, and testing here on KDS passes. We will wait for the final QA confirmation from learningequality/kolibri#12325 and then I will merge.

FYI I updated the changelog. I appreciate you started the documentation page with the example. I pushed some more updates to the documentation. Cross-referenced with KRadioButton and moved your example to KRadioButton to ensure it's clear that KRadioButton is meant to always used within KRadionButton group.

I think that as soon as learningequality/kolibri#12325 passes and there is a new KDS release with this PR merged in it, we can install the release to learningequality/kolibri#12325 and even merge it to Kolibri. There are still many other places in Kolibri where we will want to wrap KRadioButton's - I can open a follow-up issue for that.

Thanks a lot for this solid piece of work - progressing from the research, communication with @radinamatic, proof of concept, to the final implementation. So glad this is completed :)

Resolved

[MisRob]

@muditchoudhary I also added the example with grids to docs, since I don't think people would figure out this is actually supported :)!

[muditchoudhary]

Thanks a lot @MisRob for all the help and support. I'm excited too, to see this PR merging in the codebase.

Also thank you for updating the docs.

[MisRob]

QA in learningequality/kolibri#12325 passed so merging. Great work @muditchoudhary.

[muditchoudhary]

Thank you :) @MisRob