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

NcListItem lost a href with update to 7.8.1+ #3921

Closed
nickvergessen opened this issue Mar 23, 2023 · 15 comments · Fixed by #3922
Closed

NcListItem lost a href with update to 7.8.1+ #3921

nickvergessen opened this issue Mar 23, 2023 · 15 comments · Fixed by #3922
Labels
1. to develop Accepted and waiting to be taken care of bug Something isn't working feature: app-navigation Related to the app-navigation component feature: list-item Related to the list-item component regression Regression of a previous working feature
Milestone

Comments

@nickvergessen
Copy link
Contributor

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 of #3775 ?

@nickvergessen
Copy link
Contributor Author

@nickvergessen nickvergessen added bug Something isn't working 1. to develop Accepted and waiting to be taken care of feature: app-navigation Related to the app-navigation component regression Regression of a previous working feature feature: list-item Related to the list-item component labels Mar 23, 2023
@nickvergessen nickvergessen added this to the 7.8.4 milestone Mar 23, 2023
@raimund-schluessler
Copy link
Contributor

That was not intended and I think it should be fixable without reverting. Could you maybe guide me to the place where this is used in Talk?

@nickvergessen
Copy link
Contributor Author

Install Talk master or stable26, and then hover entries in the left sidebar (as per before after videos?

@nickvergessen
Copy link
Contributor Author

(observe the target URL in the bottom left)

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

Seems like the problem is here. It should use href in line 212 from RouterLink slotProps (203), not from the component prop.

https://github.com/nextcloud/nextcloud-vue/blob/4690b32d23a451c8a67d711c8066b191008f2050/src/components/NcListItem/NcListItem.vue#L202-L212

@raimund-schluessler
Copy link
Contributor

Install Talk master or stable26, and then hover entries in the left sidebar (as per before after videos?

I meant the code in Talk, I suppose it's this 🙂
https://github.com/nextcloud/spreed/blob/fc72eb92f50fd0983dd12e0498bd37063b453796/src/components/LeftSidebar/ConversationsList/Conversation.vue#L23-L33

@nickvergessen
Copy link
Contributor Author

Sorry, yes that's it

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

Usage in the Talk is correct. It passes to prop as intended. The problem is that currently NcListItem ignores the link generated by RouterLink on to prop while setting <a href.

As the result the link is not correct, though it navigate by the click handler.

I can fix it, but in an hour...

@raimund-schluessler
Copy link
Contributor

Seems like the problem is here. It should use href in line 212 from RouterLink slotProps, not from component only props.

https://github.com/nextcloud/nextcloud-vue/blob/4690b32d23a451c8a67d711c8066b191008f2050/src/components/NcListItem/NcListItem.vue#L202-L212

It wasn't like this before #3775 either. It always used the href prop. I am honestly a bit puzzled why talk ever showed a link there.

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

It wasn't like this before #3775 either. It always used the href prop. I am honestly a bit puzzled why talk ever showed a link there.

Because it is literally a link. It works as a link (it navigates), it renders as a link (<a> element).

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

Anyway, NcListItem should not render a link that navigates not to its href.

@raimund-schluessler
Copy link
Contributor

But router-link rendered as a li and the a within got its href from the prop provided. How was the href of the a ever correct?

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

But router-link rendered as a li and the a within got its href from the prop provided. How was the href of the a ever correct?

RounterLink searches a elements in slot content:

https://github.com/vuejs/vue-router/blob/3d026ea14f8f8fd93da73889d2bcd2103d44bec7/src/components/link.js#L151-L180

@raimund-schluessler
Copy link
Contributor

But router-link rendered as a li and the a within got its href from the prop provided. How was the href of the a ever correct?

RounterLink searches a elements in slot content:

https://github.com/vuejs/vue-router/blob/3d026ea14f8f8fd93da73889d2bcd2103d44bec7/src/components/link.js#L151-L180

Ah, interesting I didn't know that. I am on a fix and currently test it with talk.

@raimund-schluessler
Copy link
Contributor

Fix is in #3922.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of bug Something isn't working feature: app-navigation Related to the app-navigation component feature: list-item Related to the list-item component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants