Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate kradiobutton usages #4889

Merged

Conversation

akolson
Copy link
Member

@akolson akolson commented Jan 30, 2025

Summary

This pr wraps the KRadioButtonGroup around the KRadioButton as specified in the design docs. It also migrates the KRadioButton to use the buttonValue over the deprecated value prop.

References

Closes #4869

Reviewer guidance

  • Test all areas in studio that used the KRadioButton to ensure no regressions
    • Change language
    • Edit language (bulk editing)
    • Edit audience (bulk editing)
    • Storage request form (/settings/#/storage)
  • Run all frontend tests successfully
  • Inspect the test logs to ensure there are no warning. For reference of the warnings, see Migrate usage of KRadioButton #4869

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @akolson! I just found a little bug in the LanguageSwitcherModal, everything else looks good to me 👐.

:title="language.english_name"
class="language-name"
/>
<KRadioButtonGroup>
Copy link
Member

@AlexVelezLl AlexVelezLl Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general here I think we are managing these columns a little bit strange 😅 because on top of this we are using a grid item to render the number of columns, and for each colum we render the languages radio buttons, that means that if we add the KRadioButtonGroup inside the grid item the up and down arrows will just work for one column and not the other.

Compartir.pantalla.-.2025-02-06.09_31_02.mp4

Since KRadioButton should be a direct child of KRadioButtonGroup, I think we should refactor how we manage the column rendering to avoid having two different divs for different columns. I think grid or flex layouts would help here :) Nvm just checked it and it seems that KRadioButtonGroup does work with nested children, so we can just wrap the KGridItems with KRadioButtonGroup just as we do in Kolibri!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks @AlexVelezLl!

@akolson akolson requested a review from AlexVelezLl February 11, 2025 16:04
@AlexVelezLl AlexVelezLl self-assigned this Feb 11, 2025
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its now working fine. Thanks @akolson!

@akolson akolson merged commit 5018748 into learningequality:unstable Feb 13, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate usage of KRadioButton
2 participants