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

Links: Use browser default behavior for ctrl-clicks etc #677

Merged
merged 5 commits into from
Jun 15, 2020

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jun 10, 2020

Fixes #676

@Tobbe
Copy link
Member Author

Tobbe commented Jun 10, 2020

This is not tested at all. I'd love to write a unit test, but how do I test that the browser opened the linked page in a new tab or window? I could possibly test that redwood's navigate is not called, but I thought that might be too much of an implementation detail rely on in a test

Also haven't tested it manually, see #678 and #679

@Tobbe
Copy link
Member Author

Tobbe commented Jun 10, 2020

Manually tested Ctrl- and Shift-clicking on Windows, both work as expected.

@thedavidprice
Copy link
Contributor

Thanks for this @Tobbe! I looks good to me.

I've tested locally on Mac and can confirm the following works correctly now:

  • cmd + click opens a new tab without changing changes
  • shift + click opens in new window
  • shift + cmd + click opens a new tab an changes to new tab
  • option + click downloads HTML file

@peterp anything else you suggest here before merging?

I'd love to write a unit test, but how do I test that the browser opened the linked page in a new tab or window? I could possibly test that redwood's navigate is not called, but I thought that might be too much of an implementation detail rely on in a test

@RobertBroersma quick reactions to this case? Without EtoE I'm not sure what we could do. I’m ok to merge without unit test but wanted to check first.

@thedavidprice thedavidprice self-requested a review June 11, 2020 19:59
@RobertBroersma
Copy link
Contributor

It's a bit of an odd case to test I guess. Not sure what we could to besides checking navigate wasn't called either. I think it's fine to leave it manually tested only!

@thedavidprice thedavidprice added this to the next release milestone Jun 15, 2020
@thedavidprice thedavidprice merged commit 98df7bd into redwoodjs:master Jun 15, 2020
@Tobbe Tobbe deleted the link_ctrl_676 branch April 5, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command/Control-click on a Link doesn't open a new tab
4 participants