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: pass route extras in routerLink #2905

Merged
merged 7 commits into from
May 29, 2024

Conversation

Akshat55
Copy link
Contributor

@Akshat55 Akshat55 commented May 27, 2024

Closes #2904

Changelog

Changed

  • Now calling navigate function on anchor link click which will pass in routeExtras.

Signed-off-by: Akshat Patel <akshat@live.ca>
@Akshat55 Akshat55 requested a review from a team as a code owner May 27, 2024 15:13
Copy link

netlify bot commented May 27, 2024

Deploy Preview for carbon-components-angular ready!

Name Link
🔨 Latest commit 07c566d
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-angular/deploys/665783341c625d0008c75257
😎 Deploy Preview https://deploy-preview-2905--carbon-components-angular.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Co-authored-by: Andrea Paolucci <35496522+Licen-it@users.noreply.github.com>
@Akshat55 Akshat55 merged commit 1c1c314 into carbon-design-system:master May 29, 2024
13 checks passed
@Akshat55 Akshat55 deleted the issue.2904 branch May 29, 2024 19:44
Copy link

🎉 This PR is included in version 5.26.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@klaascuvelier
Copy link
Contributor

Hey, I am a bit late here with feedback as this MR has been merged already.

I think the current implementation effectively breaks the idea of the original change. The idea was to rely on the functionality of the directives solely to do navigation and navigation state checks, skipping any custom JS. This would allow for correct cmd + click behavior and so on.
I think the correct fix here would have been to add the navigation extras into the template where now the onclick handler was added.

@Licen-it
Copy link
Contributor

Licen-it commented Jun 4, 2024

@klaascuvelier yeah it would make sense....i didn't think about the cmd + click, but the only pain would be that then passed into the template they wouldn't be a nice since object, but each property would need to be specified separately.
I have an idea @Akshat55, but not sure if that would be ok for carbon team...we could have a directive extending RouterLink directive which is accepting a single object of type NavigationExtras and in the constructor setting each property in this object to the parent class (in a loop way so that if they add/remove any, you won't have to update this code)

@klaascuvelier
Copy link
Contributor

klaascuvelier commented Jun 5, 2024

Thanks for you reply @Licen-it.
I think adding all the navigation extras in the template of this component should not be too much work, just a one time job.
I'll draft something up quickly asap. I'll start by creating a new issue so we can take the discussion there. Thanks for collaborating on this and my apologies for breaking stuff :)

@Licen-it
Copy link
Contributor

Licen-it commented Jun 5, 2024

Thanks for you reply @Licen-it. I think adding all the navigation extras in the template of this component should not be too much work, just a one time job. I'll draft something up quickly asap. I'll start by creating a new issue so we can take the discussion there. Thanks for collaborating on this and my apologies for breaking stuff :)

Well I was mainly thinking about future proof solution, so that even if a later angular version is changing it will work fine...something to easily support different versions of angular without breaking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sidenav Item is ignoring routeExtras
3 participants