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: adjust breadcrumbs component #4416

Merged
merged 1 commit into from
Aug 23, 2023
Merged

Conversation

raimund-schluessler
Copy link
Contributor

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

☑️ Resolves

  • This PR fixes an issue with the breadcrumbs component where the getWidth function errors because it uses .elm on a vnode which might not be defined. The PR is basically a backport of the changes I did for the NcBreadcrumbs component in the vue3 branch. The PR still needs some cleanup and fine-tuning, but it seems to fix the error.

@susnux The hidden breadcumbs in the action menu currently don't work as they should. The breadcrumbs component might need to be adjusted, but I still need to figure out what's the problem.

@raimund-schluessler raimund-schluessler added 2. developing Work in progress feature: breadcrumbs Related to the breadcrumbs components labels Aug 13, 2023
@raimund-schluessler raimund-schluessler force-pushed the breadcrumbs-filepicker branch 2 times, most recently from 0b835b2 to 5273f33 Compare August 14, 2023 05:14
@raimund-schluessler
Copy link
Contributor Author

@susnux The hidden breadcumbs in the action menu currently don't work as they should. The breadcrumbs component might need to be adjusted, but I still need to figure out what's the problem.

This is partially fixed by second commit in the PR now. The hidden crumbs now correctly execute the bound listeners and navigating directories in the filepicker now also works for hidden crumbs. However, clicking a hidden crumb also navigates in the browser and the URL changes, which should not be the case. Maybe this can be skipped with a preventDefault on the listener, but I think adjusting NcBreadcrumbs to not navigate at all in such cases would be cleaner.

@raimund-schluessler
Copy link
Contributor Author

However, clicking a hidden crumb also navigates in the browser and the URL changes, which should not be the case. Maybe this can be skipped with a preventDefault on the listener, but I think adjusting NcBreadcrumbs to not navigate at all in such cases would be cleaner.

@susnux I think the issue is that the hidden crumbs always render to an ActionLink or ActionRouter which makes the browser navigate. How about we allow also ActionButton in case the crumb has neither a to nor href set? I think we never really considered using NcBreadcrumbs in a non navigating context, like it's done in the Filepicker now.

@raimund-schluessler
Copy link
Contributor Author

@susnux I think the issue is that the hidden crumbs always render to an ActionLink or ActionRouter which makes the browser navigate. How about we allow also ActionButton in case the crumb has neither a to nor href set? I think we never really considered using NcBreadcrumbs in a non navigating context, like it's done in the Filepicker now.

I implemented it like this now. Let me know whether this fixes all issues you saw with the NcBreadcrumbs component in the filepicker. For me, everything works now. Crumbs get dynamically hidden on resize and navigating with hidden crumbs works fine now. If the fix works for you as well, I will cleanup a bit so we can merge it after review.

@raimund-schluessler
Copy link
Contributor Author

@susnux @skjnldsv I know you sure are busy with 27.1, but it would be great if you could have a look at this PR here, and check whether it fixes the issues with the breadcrumb bar in the new vue filepicker component. I think this should also be included for 27.1. because it otherwise could be seen as a regression.

@skjnldsv
Copy link
Contributor

I know you sure are busy with 27.1, but it would be great if you could have a look at this PR here, and check whether it fixes the issues with the breadcrumb bar in the new vue filepicker component. I think this should also be included for 27.1. because it otherwise could be seen as a regression.

Looks good to me! :)
Added one comment fixing a height issue

@raimund-schluessler
Copy link
Contributor Author

Looks good to me! :) Added one comment fixing a height issue

Thanks for having a look. Could you check whether it fixes the issue with the crumbs in the filepicker for you? Anyway, I will cleanup the PR a bit and once @susnux had a look, we can merge it.

@skjnldsv
Copy link
Contributor

Thanks for having a look. Could you check whether it fixes the issue with the crumbs in the filepicker for you? Anyway, I will cleanup the PR a bit and once @susnux had a look, we can merge it.

Seems to work for me. No more error messages :)

@raimund-schluessler
Copy link
Contributor Author

I cleaned up a bit. After a rebase and a squash this should be good to go.

@susnux susnux marked this pull request as ready for review August 23, 2023 19:49
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Works perfectly now! Thank you 🎉

Just squash and then ready to go :)

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler merged commit 50b5f2c into master Aug 23, 2023
16 checks passed
@raimund-schluessler raimund-schluessler deleted the breadcrumbs-filepicker branch August 23, 2023 20:18
@raimund-schluessler raimund-schluessler restored the breadcrumbs-filepicker branch August 23, 2023 20:20
@raimund-schluessler raimund-schluessler modified the milestones: 7.12.3, 8.0.0 Aug 23, 2023
@raimund-schluessler raimund-schluessler added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 23, 2023
@skjnldsv skjnldsv added the bug Something isn't working label Aug 24, 2023
@raimund-schluessler raimund-schluessler deleted the breadcrumbs-filepicker branch August 25, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish 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.

3 participants