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(angular RouterLinkDelegateDirective): Make routerLink respect target and keyboard modifiers #28976

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

j-oppenhuis
Copy link

Issue number: resolves #26394


What is the current behavior?

This fixes using routerLink in combination with a target different than _self. It also fixes a bug where CTRL + click or shift + click does not work like expected in combination with routerLink in Angular.

What is the new behavior?

When using a target other than _self and keyboard modifiers we are now stopping execution of navigating by routeDirection. This is not needed because the app will reload either way.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: angular @ionic/angular package label Feb 5, 2024
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR to the project! We appreciate your contribution.

There are a few things that need to be addressed before we can move forward with the PR.

  1. Command clicking is not following expected browser behavior. While I’m able to CMD + click on the button and the link opens in a new tab, the active page navigates to the same url.
  2. The OPTION key is not being taken into account when clicking on the button if the user is using a Mac. The user shouldn't be able to open the link in a new tab and the page should not redirect to the link passed to routerLink.
  3. As a noted in the first item, the page should not redirect to the link passed to routerLink. The redirection is happening because of a behavior that is implemented in ion-button. We would want to prevent Ionic's click listener from triggering when a new tab is opened.
  4. We also want to check the operating system of the user to tie the correct key to the correct action. The OPTION key should only be taken into account when the user is using a Mac.

Let me know if you have any questions! Thank you for your time and effort!

@j-oppenhuis
Copy link
Author

Thank you for your feedback. I understand that it is more complex than my initial idea. Instead of changing routerLink functionality, we should probably change the openURL function. Am I correct? If that is the case than we should indeed also pass in the operating system and the target. But is that a good solution?

@j-oppenhuis
Copy link
Author

I have looked into the code and I think my suggestion is incorrect. But it seems we are not able to prevent the button or item navigation by using stoppropagation. The only thing I can think of is adding a routerLink prop to the button and item components. Then we could make a distinction between desktop and mobile. When the current OS is desktop, the anchor could handle navigation. Otherwise, we could use openURL like we use now. Is that something that could work?

@thetaPC
Copy link
Contributor

thetaPC commented Apr 1, 2024

We've checked out the problem, and it's a tough one. But because we're short on time, our team can't really dig into it right now. However, we're up for bouncing around ideas together. We might not have all the answers, but we'll do what we can to help out.

I agree that passing an operating system is not the way to go. We want ion-button to not have strict checks like what OS the user has.

@sean-perkins sean-perkins removed their request for review April 26, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package
Projects
None yet
2 participants