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

fix(NcListItem): Bring back correct href for router-link links #3922

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Mar 23, 2023

This brings back the correct href for the link in NcListItem and NcAppNavigationItem that got erroneously removed with #3775. Closes #3921.

Tested with Talk master and the links show up again.

@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews 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
@raimund-schluessler raimund-schluessler added this to the 7.8.4 milestone Mar 23, 2023
@raimund-schluessler raimund-schluessler marked this pull request as draft March 23, 2023 21:10
@raimund-schluessler
Copy link
Contributor Author

Still needs a tweak as links now open in a new tab by default.

@raimund-schluessler raimund-schluessler force-pushed the fix/3921/missing-href branch 2 times, most recently from 254c968 to 1dd7b68 Compare March 23, 2023 21:15
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.

It seems, we don't use snake_case for variable names.

href_router -> routerLinkHref ?

@raimund-schluessler
Copy link
Contributor Author

Still needs a tweak as links now open in a new tab by default.

Fixed. target and rel only took the hrefprop into account and not the href coming from router-link.

However, the link now behaves as a normal link and reloads the page. Still needs fixing.

@raimund-schluessler
Copy link
Contributor Author

It seems, we don't use snake_case for variable names.

href_router -> routerLinkHref ?

Fine with me.

@ShGKme
Copy link
Contributor

ShGKme commented Mar 23, 2023

However, the link now behaves as a normal link and reloads the page. Still needs fixing.

@click="onClick" -> @click.prevent="onClick" ?

@raimund-schluessler
Copy link
Contributor Author

However, the link now behaves as a normal link and reloads the page. Still needs fixing.

@click="onClick" -> @click.prevent="onClick" ?

Yes, but only if it's a router-link a. I test it right now, but building Talk takes ages here 🙈

@raimund-schluessler
Copy link
Contributor Author

Seems to work as it should now.

@raimund-schluessler raimund-schluessler marked this pull request as ready for review March 23, 2023 22:05
@raimund-schluessler
Copy link
Contributor Author

I checked it with Talk and Tasks now and both NcListItem and NcAppNavigationItem work correctly now.

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Code make sense

Copy link
Contributor

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Fixes talk again and the explanations make sense

@nickvergessen nickvergessen merged commit 35e2fc9 into master Mar 24, 2023
@nickvergessen nickvergessen deleted the fix/3921/missing-href branch March 24, 2023 13:40
@nickvergessen nickvergessen changed the title Bring back correct href for router-link links fix(NcListItem): Bring back correct href for router-link links Mar 24, 2023
@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

@raimund-schluessler I just noticed that there might a bug in this PR:

routerLinkHref is not defined in the NcListItem component. It is only used in some places. Could you elaborate on where the item is coming from?

$ rg routerLinkHref src/components/NcListItem/NcListItem.vue
25:             v-slot="{ href: routerLinkHref, navigate, isActive }"
33:                             :href="routerLinkHref || href"
43:                             @click="onClick($event, navigate, routerLinkHref)"
299:            onClick(event, navigate, routerLinkHref) {
304:                    if (routerLinkHref) {

PS: I'm currently trying to debug a major regression from #3775 (comment)

@ShGKme
Copy link
Contributor

ShGKme commented Mar 30, 2023

routerLinkHref is not defined in the NcListItem component. It is only used in some places. Could you elaborate on where the item is coming from?

routerLinkHref don't have to be defined in NcListItem. It is renamed slot prop (aka slot rendering function parameter). This code is close to this JS equivalent:

const href // href from prop
function renderSlot({ href: routerLinkHref, navigate, isActive  } = {}) {
  return h('a', { attrs: { href: routerLinkHref || href } })
}

When is === RouterLink, v-slot takes href value from the Routerlink, then it is renamed as function param to routerLinkHref and used in rendering.

When is === 'a', v-slot takes undefined, routerLinkHref is undefined and routerLinkHref || href uses href component prop.

@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

That makes sense. Thanks for the explanation.

I found the problem: to.exact is not honored. Instead, I have to provide the :exact prop manually.

Ref https://v3.router.vuejs.org/api/#exact

@st3iny
Copy link
Contributor

st3iny commented Mar 30, 2023

From my perspective this is a breaking change because it was honored in 7.8.0 and we should fix it.

It turns out that exact is a prop of router-link and a fix in mail is sufficient. I have no idea how it worked at all before 😅

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 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 this pull request may close these issues.

NcListItem lost a href with update to 7.8.1+
5 participants