fix(angular): allow default browser action on CTRL+Click #24440
Conversation
|
|
Hello @Juarrow thanks for this PR! You do not need to commit I think maintaining the existing |
sean-perkins
changed the title
| if (!this.elementRef.nativeElement.closest('ion-button')) { | ||
| // In case of CTRL+Click or Meta+Click, | ||
| // ignore those to allow default browser actions. | ||
| if (this.platform.is('desktop') && ev instanceof MouseEvent) { |
There was a problem hiding this comment.
Why do we need to check if the device is on desktop/a MouseEvent?
There was a problem hiding this comment.
Agreed this appears to be redundant. The native click event is a MouseEvent. I'd advocate we remove the platform check to additionally handle tablet with a connected keyboard & mouse.
There was a problem hiding this comment.
Oftentimes, I tend to be too defensive with all the checks.
Will remove the if-statement.
There was a problem hiding this comment.
@Juarrow let me know when you have a moment to update the PR with this change.
There was a problem hiding this comment.
@sean-perkins just did. Also updated the commit message scope to reflect your change to the PR title.
There was a problem hiding this comment.
@Juarrow this task is in my sprint to both finish out and address this PR. The main issue effects each framework output. After looking deeper at ion-button, I would like to address to inconsistencies with that component (no action required on your end).
Here's my WIP branch: https://github.com/ionic-team/ionic-framework/tree/FW-432 that pulls in your Angular change and applies a similar resolution to React and Vue.
Once I am able to diagnose the ion-button behavior fully and add respective tests, I will likely open a new PR and give you co-author credit on the PR.
Juarrow
force-pushed
the
24413-router_link_ctrl_click
branch
from
1c19d0d
to
6f110c2
Compare
|
Closing this PR in favor of: #25014. I did give you co-authored-by credit for the commit. Appreciate the help here! Community PR contributions is still an area we are actively trying to improve on. We are trying to tackle the older PRs first, as they are often times the hardest PRs to evaluate. Our time to reply can definitely be improved 👍 |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: #24413
What is the new behavior?
Does this introduce a breaking change?
Other information