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 from router-link's tag prop to v-slot #3775

Merged
merged 4 commits into from Mar 10, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Feb 17, 2023

This PR migrates the NcAppNavigationItem, NcButton and NcListItem components from using the dreprecated tag prop of router-link to the v-slot API introduced with vue-router@3.1.0, see https://v3.router.vuejs.org/api/#v-slot-api-3-1-0. This is required to make the library compatible with vue-router@4 for vue 3 in #3692.

There are no functional changes and the markup stays the same. For the dev, nothing should change, besides that the corresponding warning from vue-router is gone.

The PR is best viewed as https://github.com/nextcloud/nextcloud-vue/pull/3775/files?diff=unified&w=1

@raimund-schluessler raimund-schluessler added 2. developing Work in progress feature: app-navigation Related to the app-navigation component technical debt feature: list-item Related to the list-item component labels Feb 17, 2023
@raimund-schluessler raimund-schluessler force-pushed the fix/noid/router-link-slot branch 5 times, most recently from 601b375 to 2df8e5c Compare February 24, 2023 15:59
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler changed the title Migrate from router-link tag prop to v-slot Migrate from router-link's tag prop to v-slot Feb 25, 2023
@raimund-schluessler raimund-schluessler linked an issue Feb 25, 2023 that may be closed by this pull request
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 25, 2023
@raimund-schluessler raimund-schluessler marked this pull request as ready for review February 25, 2023 17:04
@Chartman123
Copy link
Contributor

@raimund-schluessler I pulled the latest code from this branch and linked it to my forms dev setup. The router warning is indeed gone. Howevere, I got some other errors now. It's also possible that I did something wrong.

[Vue warn]: $attrs is readonly.

found in

---> <VPopperContent>
       <VPopper>
         <VDropdown>
           <NcPopover>
             <NcActions>
               <RouterLink>
                 <NcListItem>
                   <AppNavigationForm> at src/components/AppNavigationForm.vue
                     <NcAppNavigation>
                       <NcContent>
                         <Forms> at src/Forms.vue
                           <Root> forms-main.js:86037:21

the message repeats for $attrs and $listeners

@raimund-schluessler
Copy link
Contributor Author

@Chartman123 thanks for testing the PR. The warnings you see are unrelated to the changes here and are due to npm link, see #3281. We fixed the problem in nextcloud-libraries/webpack-vue-config#432 but that’s not released yet.
If I recall correctly, it also helped to build both nextcloud/vue and the app in development mode.

@nickvergessen nickvergessen removed their request for review February 27, 2023 09:29
@nickvergessen
Copy link
Contributor

Not my expertise, sorry

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler added this to the 7.8.0 milestone Feb 28, 2023
@julien-nc julien-nc modified the milestones: 7.8.0, 7.9.0 Mar 2, 2023
@raimund-schluessler
Copy link
Contributor Author

Now that 7.8.0 is out, would anyone have time reviewing this, so I can proceed with #3692?

@raimund-schluessler raimund-schluessler mentioned this pull request Mar 4, 2023
2 tasks
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.

All the components looks good and checked.
Thanks for this work!
Especially for removing <nav-element v-bind="{ is }"> 😀

@raimund-schluessler raimund-schluessler merged commit e62a8e3 into master Mar 10, 2023
@raimund-schluessler raimund-schluessler deleted the fix/noid/router-link-slot branch March 10, 2023 19:38
@raimund-schluessler raimund-schluessler modified the milestones: 7.9.0, 7.8.1 Mar 11, 2023
@julien-nc julien-nc mentioned this pull request Mar 15, 2023
@nickvergessen
Copy link
Contributor

nickvergessen commented Mar 23, 2023

This is actually a breaking change:
nextcloud/spreed#9153

NcListItem entries like Talk's conversations in the left sidebar lost their a href and can therefore no longer be opened in a new tab with the mouse wheel, ctrl + click or right-click option, instead of the home page of talk or current conversation is loaded again with a trailing #

Before After
Bildschirmaufzeichnung vom 2023-03-23, 20-53-01.webm Bildschirmaufzeichnung vom 2023-03-23, 20-51-35.webm

Is that fixable without a revert?

@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

Unfortunately, this PR completely breaks mail because :href is always overwriting :to for some reasons and NcListItem :to links don't work anymore.

@raimund-schluessler
Copy link
Contributor Author

Unfortunately, this PR completely breaks mail because :href is always overwriting :to for some reasons and NcListItem :to links don't work anymore.

Did you check it with #3922 included?

@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

Unfortunately, this PR completely breaks mail because :href is always overwriting :to for some reasons and NcListItem :to links don't work anymore.

Did you check it with #3922 included?

Yes, I already did (latest master). Also see my comment there.

@ShGKme
Copy link
Contributor

ShGKme commented Mar 30, 2023

Unfortunately, this PR completely breaks mail because :href is always overwriting :to for some reasons and NcListItem :to links don't work anymore.

Do you provide both to and href props?
Could you send a link to the problem place or issue?

@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

Do you provide both to and href props? Could you send a link to the problem place or issue?

I provide only :to={...some object...}. It always worked this way as expected. Now, it is broken.

https://github.com/nextcloud/mail/blob/3f6f240b61f6b4b4bc89ac72c8bf20f0e617eec1/src/components/Envelope.vue#L1-L18

PS: I also tried to explicitly set href="" but that doesn't fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component feature: button feature: list-item Related to the list-item component technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation warning about tag prop of router-link
6 participants