Skip to content

Conversation

@janbaum
Copy link
Contributor

@janbaum janbaum commented Oct 9, 2025

Fixes: #7544

๐Ÿ–ผ๏ธ Screenshots

๐Ÿš๏ธ Before ๐Ÿก After
image image

๐Ÿ Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits

@mejo- mejo- added bug Something isn't working 3. to review labels Oct 9, 2025
@janbaum janbaum added bug Something isn't working 3. to review labels Oct 9, 2025
@codecov
Copy link

codecov bot commented Oct 9, 2025

Codecov Report

โœ… All modified and coverable lines are covered by tests.
โœ… Project coverage is 59.98%. Comparing base (a691728) to head (f4cfc69).
โš ๏ธ Report is 120 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7750      +/-   ##
==========================================
+ Coverage   59.27%   59.98%   +0.70%     
==========================================
  Files         504      312     -192     
  Lines       39233    38372     -861     
  Branches     1137      945     -192     
==========================================
- Hits        23257    23019     -238     
+ Misses      15868    15245     -623     
  Partials      108      108              

โ˜” View full report in Codecov by Sentry.
๐Ÿ“ข Have feedback on the report? Share it here.

๐Ÿš€ New features to boost your workflow:
  • โ„๏ธ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • ๐Ÿ“ฆ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Oct 10, 2025

@janbaum Thanks a lot for your PR!

The linter is unhappy. I think the message is self explanatory:
image

Could you try and fix this?

After fixing you can confirm with npm run lint that your fix worked and did not trigger anything else.

Feel free to amend the existing commit (git commit --amend --no-edit src/components/Editor/PreviewOptions.vue) and force push it to your branch.

update: Just noticed that the error is maybe not so self-explanatory. So I will elaborate a bit.

v-model is a two way binding - it sets the value in the child component and when the child emits an event it changes it in the parent.

But here type is a prop and props are not expected to be changed. We already handle the @change event for the toggles. So instead of the two way binding you only want a one way binding.

The checked prop that we used before is deprecated:
https://github.com/nextcloud-libraries/nextcloud-vue/blob/d747fe3358cdb0a84aa409686ebdd73595c32316/src/components/NcActionRadio/NcActionRadio.vue#L101-L119

Recommendation is to use modelValue instead - which is also what v-model uses under the hood for providing the value to the child component ( so the one way we still need ).

@mejo-
Copy link
Member

mejo- commented Oct 10, 2025

Thanks for the comments @max-nextcloud! We actually struggled with this problem while trying to find a proper fix. But you're right, as documented here, component props in Vue.js must not be mutated inside the component.

So since we still have to use v-model for the NcActionRadio component, we probably have to introduce a local component property in the data() object of the component (e.g. selectedType) and set it to the value of the type component prop in created().

Me and @janbaum actually already did this first while working on the problem yesterday, but I didn't like it because it left us with a duplicated data structure. But apparently that's the way to go then, no?

@janbaum could you imagine to update the PR accordingly?

@max-nextcloud
Copy link
Collaborator

@mejo- I think the crucial part of your fix is:

  • that v-model uses modelValue rather than checked internally.
  • that you hand it a string rather than a boolean so it can do the comparison with the value itself.

I think you could still achieve both without using v-model by setting the modelValue prop to type. But I have not tried it though.

I also considered the data option you suggest and thought it was overly complex. But if that's the only way to go that's also fine with me.

@janbaum
Copy link
Contributor Author

janbaum commented Oct 11, 2025

@mejo- @max-nextcloud Thank you both for the hints, I'll try :)

@max-nextcloud
Copy link
Collaborator

Nice! Confirmed it works. Thanks a lot! โค๏ธ

@max-nextcloud
Copy link
Collaborator

/backport to stable32

@max-nextcloud
Copy link
Collaborator

/backport to stable31

@max-nextcloud max-nextcloud merged commit 16d0134 into nextcloud:main Oct 13, 2025
63 of 64 checks passed
@backportbot
Copy link

backportbot bot commented Oct 13, 2025

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31

# Create the new backport branch
git checkout -b backport/7750/stable31

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick f4cfc694

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/7750/stable31

Error: Failed to check for changes with origin/stable31: No changes found in backport branch


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@max-nextcloud
Copy link
Collaborator

@janbaum The fix has been merged and backported to Nextcloud 32. The backport to 31 failed and needs to be done by hand.

Would you have some time until Wednesday to give this a try? Otherwise we'll take care of it to make sure it lands in the next patch release.

@janbaum
Copy link
Contributor Author

janbaum commented Oct 13, 2025

@janbaum The fix has been merged and backported to Nextcloud 32. The backport to 31 failed and needs to be done by hand.

Would you have some time until Wednesday to give this a try? Otherwise we'll take care of it to make sure it lands in the next patch release.

Tbh I am not sure if its correct what I did :D

#7781

@max-nextcloud
Copy link
Collaborator

Tbh I am not sure if its correct what I did :D

Thanks a lot for giving it a try! mejo already got back to you. If you need further advice feel free to mention me in the new PR.

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link preview state toggle is not rendered correctly

3 participants