-
Notifications
You must be signed in to change notification settings - Fork 188
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
Migrate kradiobutton usages #4889
Conversation
There was a problem hiding this 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> |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks @AlexVelezLl!
There was a problem hiding this 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!
Summary
This pr wraps the
KRadioButtonGroup
around theKRadioButton
as specified in the design docs. It also migrates theKRadioButton
to use thebuttonValue
over the deprecatedvalue
prop.References
Closes #4869
Reviewer guidance
/settings/#/storage
)KRadioButton
#4869