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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

feat(NcCheckboxRadioSwitch): add v-model support for checked #5418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(NcCheckboxRadioSwitch): add v-model support for checked #5418

wants to merge 1 commit into from

Conversation

kyteinsky
Copy link
Contributor

Can we get this in nc/vue 8.x or does it not matter since it was added in vue9 recently (#4994)? Would be a nice QOL improvement still.

馃弫 Checklist

  • 鉀戯笍 Tests are included or are not applicable
  • 馃摌 Component documentation has been extended, updated or is not applicable
  • 3锔忊儯 Backport to next requested with a Vue 3 upgrade

ShGKme
ShGKme reviewed Mar 17, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I think, that would be good to add model to every component that has modelValue in the next version.

Just to make further migration to v9 easier for library users.

All these components from the Breaking Changes list where it doesn't conflict with the current implementation.

https://github.com/nextcloud-libraries/nextcloud-vue/blob/next/CHANGELOG.md

@susnux susnux added enhancement New feature or request 2. developing Work in progress feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Mar 20, 2024
@susnux susnux added this to the 8.12.0 milestone Mar 20, 2024
@ShGKme
Copy link
Contributor

ShGKme commented Apr 9, 2024

Hey @kyteinsky!

What do you think about adding v-model to all these components?

@kyteinsky
Copy link
Contributor Author

Hello @ShGKme , sorry I didn't find time for it recently. Yeah, looks like a sound reasoning.

Is it alright if I do that the next week?

@ShGKme
Copy link
Contributor

ShGKme commented Apr 10, 2024

Is it alright if I do that the next week?

Sure

@susnux susnux modified the milestones: 8.12.0, 8.13.0 May 19, 2024
@kyteinsky
feat: add v-model to remaining input components
eb74cc9 
This facilitates migration from Vue 2 to Vue Next (3) easier.
Also comes with the v-model advantages like the number modifier
(v-model.number) which is not possible with .sync prop modifier.

Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the feat/v-model-checkbox-radio branch from 1e5f927 to eb74cc9 Compare May 22, 2024 01:46
@kyteinsky
Copy link
Contributor Author

Some components did not need modification and already work with v-model:

  1. NcColorPicker
  2. NcDateTimePicker
  3. NcDateTimePickerNative
  4. NcSelect
  5. NcSelectTags
  6. NcSettingsSelectGroup
  7. NcTimezonePicker

NcActionRadio does not make use of v-model in any useful way so I left it out. Enlighten me if I'm wrong here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme left review comments

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
2. developing Work in progress enhancement New feature or request feature: checkbox-radio-switch Related to the checkbox-radio-switch component
Projects
None yet
Milestone
8.13.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kyteinsky @ShGKme @susnux