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

Updated KRadioButton 'value' prop #485

Merged
merged 21 commits into from
Dec 11, 2023
Merged

Conversation

BabyElias
Copy link
Contributor

@BabyElias BabyElias commented Oct 31, 2023

Description

Updated the KRadioButton 'value' prop to 'buttonValue' and configured it to avoid confusion regarding API decision.

Issue addressed

Addresses #379

Before/after screenshots

Before
image
After
image

Changelog

  • #485
    • Description: Updated KRadioButton 'value' prop to 'buttonValue'
    • Products impact: Updated API
    • Addresses: KRadioButton redundant v-model / value #379
    • Components: KRadioButton
    • Breaking: Yes
    • Impacts a11y:
    • Guidance: KRadioButton 'value' prop is deprecated. Please use the 'buttonValue' prop instead.

@BabyElias
Copy link
Contributor Author

BabyElias commented Oct 31, 2023

The asked change, caused the frontend test to fail. So, have reverted it for now.
Edit : The error is related to KRresponsiveWindow component and not KRadioButton, so applied the change again

@LianaHarris360
Copy link
Member

@BabyElias Yes, the failing test was not caused by the changes you made. It is an issue that will be addressed separately. Thank you for your contributions! I will now open the related issues in the other Product repositories and can approve this PR once it is verified whether the other repositories are using the deprecated value prop in KRadioButton.

In case it takes several days, please note that the timeframe for the approval and merging of this pr is unknown due to other priorities within those repositories.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Rather than just replacing value altogether, we need to more gradually change the behavior because all of the places using this component in other code bases will be using value as their prop.

If we change things so that using value doesn't work then we would force those projects to adhere to this change immediately in order to use the latest KDS version. In order to provide more flexibility for the rest of our team as we transition, we need to accommodate both of the props for now.

In a future release, we will finally remove the value prop altogether which will be indicated in that release's changelog and we can basically just apply these changes as-is because if we didn't need to more gradually deprecate it, these changes would be spot-on. Thank you for your work on this and I apologize for not being more explicit and clear about what was needed in the issue.


Requested changes:

  • Allow use of either of the props value or buttonValue
  • In the component's mounted() hook, if the value prop is not undefined put a console.warn message "KRadioButton prop value is deprecated and will be removed in a future release. Please use buttonValue in its place."
  • Because the props will both have to be required:false, add a console.error message in the mounted() hook "KRadioButton: buttonValue prop is required". No need to mention the value prop here.

@BabyElias
Copy link
Contributor Author

Hey!
This is just to update that I am still working on this PR and trying to find the best implementation for this deprecation to be brought about. I have been a bit busy lately due to my exams, but I'll try to create a PR as soon as possible.
Thank you for your patience and time :)

@LianaHarris360
Copy link
Member

No need to worry at all, thank you for keeping us updated and we wish you the best of luck on your exams!

@BabyElias
Copy link
Contributor Author

BabyElias commented Nov 30, 2023

Hey @LianaHarris360 & @nucleogenesis, I am done with all the required changes.

  • Added support for use of both props 'value' and 'buttonValue'
  • Added mounted() hook with console.warn and console.error messages

PS : One of the lint related PR checks is failing, I am not sure what's causing it. Looking into it right now.

Copy link
Member

@LianaHarris360 LianaHarris360 left a comment

Choose a reason for hiding this comment

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

Overall the code changes look good! The failing lint checks can be fixed by running yarn run lint-frontend:format. More info on that can be found here. I also left some comments below for a few changes that will help avoid potential issues with boolean values.

@BabyElias
Copy link
Contributor Author

Made all the required changes.
About the linting error, the command yarn run lint-frontend:format did not exist in the package.json file and hence gave errors.
I tried out other linting commands that were available like yarn lint-fix and yarn lint-watch but they just show this
Kolibri Linter: Initializing watcher for the following patterns: '**/*.{js,vue,scss,less,css}' and nothing happens.
Manually linted the entire file too, but it seems to have failed the check still :(

@nucleogenesis
Copy link
Member

nucleogenesis commented Dec 6, 2023

@BabyElias that's strange -- it could take a long time because it will be going over all of the files but it shouldn't take so long that it feels like it might be broken 😞

If you try yarn install --force then yarn lint-fix again, perhaps it is an issue with your currently installed version of kolibri-tools (where the linter lives).

In any case -- if you merge this PR I made on your fork targeting your branch then this PR should receive the fixes and pass the checks.

Hope any of this helps! Thanks for all of your work on this !

@BabyElias
Copy link
Contributor Author

@nucleogenesis Thank you so much for the targeted PR ! It finally helped pass the linting checks.
PS : I did the yarn install --force and yarn lint-fix again, this time the Linter showed as "Done in 6.2 secs" but did not change anything in my files. Maybe, I'll have to check something with my local settings.
image

@MisRob MisRob requested a review from LianaHarris360 December 7, 2023 14:33
@MisRob MisRob dismissed nucleogenesis’s stale review December 11, 2023 07:36

Feedback seems to be addressed

@MisRob MisRob merged commit 795e909 into learningequality:main Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KRadioButton redundant v-model / value
4 participants