Skip to content

Conversation

@raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Apr 13, 2023

We currently wrongfully overwrite the href attribute of the router-link in NcBreadcrumb with the default href prop value. This makes the link rendered by router-link to not have its href attribute correctly set, which prevents the default link behavior. Navigating still works though, so I guess this is why we didn't notice yet. This PR fixes it by only setting the href attribute from the prop if it's not a router-link.

@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews feature: breadcrumbs Related to the breadcrumbs components labels Apr 13, 2023
@raimund-schluessler raimund-schluessler added this to the 7.9.1 milestone Apr 13, 2023
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.

Instead of fixing it in the NcBreadcrumb, shouldn't we fix it in NvBreadcrumbs, where wrong props are passed?

https://github.com/nextcloud/nextcloud-vue/blob/f46bba55b191d25497878c41e00c1feb42ab36d4/src/components/NcBreadcrumbs/NcBreadcrumbs.vue#L552-L572

@raimund-schluessler
Copy link
Contributor Author

Instead of fixing it in the NcBreadcrumb, shouldn't we fix it in NvBreadcrumbs, where wrong props are passed?

https://github.com/nextcloud/nextcloud-vue/blob/f46bba55b191d25497878c41e00c1feb42ab36d4/src/components/NcBreadcrumbs/NcBreadcrumbs.vue#L552-L572

The lines you show here are not related to the problem. They are for showing the hidden crumbs in a dropdown menu. NcBreadcrumb is used explicitly by the dev and hence definitely needs a fix. We can of course, as a follow-up, also adjust these lines to not pass href to NcActionRouter, but NcActionRouter does not care if its provided an undefined href, so I am not sure it's worth the effort.

@raimund-schluessler raimund-schluessler force-pushed the fix/noid/breadcrumb-router-href branch from 5b3fde0 to 7511533 Compare April 13, 2023 09:35
@ShGKme
Copy link
Contributor

ShGKme commented Apr 13, 2023

The lines you show here are not related to the problem. They are for showing the hidden crumbs in a dropdown menu.

Yep, sorry, didn't look carefully

@raimund-schluessler raimund-schluessler merged commit 0b66d10 into master Apr 13, 2023
@raimund-schluessler raimund-schluessler deleted the fix/noid/breadcrumb-router-href branch April 13, 2023 12:11
@Pytal Pytal mentioned this pull request Apr 17, 2023
@AndyScherzinger AndyScherzinger modified the milestones: 7.9.1, 7.10.0 Apr 18, 2023
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: breadcrumbs Related to the breadcrumbs components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants