feat(breadcrumb): added href and stopped event default on a element #1169
Conversation
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
emoral435
added
bug
emoral435
requested review from
susnux,
JuliaKirschenheuter,
skjnldsv,
Pytal and
ShGKme
|
As a follow up - as I prevented the default action for this a element, it therefor has become disabled, however, we will use it as a button. While this works, would it not make more sense to make each element a NcButton element? |
|
/backport to stable28 |
| /** | ||
| * Gets the current url path | ||
| */ | ||
| const getPath = computed(() => window.location.pathname) |
There was a problem hiding this comment.
Note: window.location.pathname is not reactive. So computed won't recalculate on pathname change. If the location is not supposed to be changed, it can be a plain constant. If it can change, it should be a method (it is called as a method already).
There was a problem hiding this comment.
But speaking about the issue, I don't think it should be a link at all. With this implementation, a user has a list of links, all to the current page.
Probably, NcBreadcrumb should support rendering as a button when non to and href is provided.
cc @susnux and @JuliaKirschenheuter for a second opinion here.
There was a problem hiding this comment.
I agree with your second comment, rendering it as a link does not make any sense in this case.
There was a problem hiding this comment.
Yes, in this case, the way that these breadcrumbs are handled are that they each emit an event to reload the popup window, but it does not redirect the user to a different URL! So, I do agree, I think because the href in the breadcrumbs is essentially useless, we should change this to a list of buttons that have the same emit
| @@ -3,7 +3,8 @@ | |||
| <template #default> | |||
| <NcBreadcrumb :name="t('Home')" | |||
| :title="t('Home')" | |||
| @click="emit('update:path', '/')"> | |||
| :href="getPath" | |||
There was a problem hiding this comment.
I do not think this makes sense, this will be the current app which might be something other than files
|
Closing PR - listened to feedback, and decided that it would be better to render |
☑️ For nextcloud/server#42624
🖼️ Screenshots
GIF For New Changes
🚧 Summary
Added an HREF attribute to the link element. Once this gets merged / challenged / changed, will update the library in nextcloud/server