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.

Sign up for GitHub

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

Jump to bottom

Only handle "normal" clicks on <Link>s #3056

Merged
merged 2 commits into from Dec 22, 2022
Merged

Only handle "normal" clicks on <Link>s #3056

merged 2 commits into from Dec 22, 2022

Conversation

kaisalmon
Copy link
Contributor

@kaisalmon kaisalmon commented Dec 22, 2022 •
edited

Description

This issue fixes Issue 2911

It uses the exact same approach as React Router

Yew Router's Link's onclick now early returns if any of the following keys are held:

  • Control: this allows links to be opened in new tabs on windows machines
  • Meta (aka Mac command key): this allows links to be opened in new tabs on macs
  • Alt: This allows users to open a link in the save dialog
  • Shift: This allows users to open al in a new window

Fixes #2911

Checklist

  • cargo make test-flow passes
  • cargo make lint passes
  • I have reviewed my own code
  • I have verified that the examples work as intended
  • I have added tests - The onclick function was entirely untested. I am happy to refactor the logic into a stand-alone function and run a test on that. I am also happy to be introduced to a fancy way of testing the on-click browser behaviours

@kaisalmon
Fixing yewstack#2911
452223a 
Prevents Link onclick behaviour from executing if the Ctrl key (for
Windows and Linux) or Meta Key (For Mac) is pressed.

This technically introduces a bug that means that links will reload the
page on windows machines when the windows key is held down. However,
this error is also in React Router, so I think we can get away with it.

See:
https://github.com/remix-run/react-router/blob/11156ac7f3d7c1c557c67cc449ecbf9bd5c6a4ca/packages/react-router-dom/dom.ts#L29
@kaisalmon
Router Links now use default browser behaviour for alt and shift keys.
fb534f4 
This change is inspired by https://github.com/remix-run/react-router/blob/11156ac7f3d7c1c557c67cc449ecbf9bd5c6a4ca/packages/react-router-dom/dom.ts#L2://github.com/remix-run/react-router/blob/11156ac7f3d7c1c557c67cc449ecbf9bd5c6a4ca/packages/react-router-dom/dom.ts#L29

This allows uses to shift click links to save whatever the link points
at, and alt click on links to open them in new windows
@github-actions
Copy link

github-actions bot commented Dec 22, 2022 •
edited

Visit the preview URL for this PR (updated for commit fb534f4):

https://yew-rs-api--pr3056-issues-2911-mn4xsbe3.web.app

(expires Thu, 29 Dec 2022 14:30:21 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Dec 22, 2022 •
edited

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 376.856 376.993 376.913 0.054
Hello World 10 707.811 721.820 714.490 5.011
Function Router 10 2398.048 2417.552 2409.271 6.313
Concurrent Task 10 1007.874 1008.921 1008.500 0.395

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 371.990 372.473 372.161 0.154
Hello World 10 703.557 721.087 707.183 5.970
Function Router 10 2396.404 2409.581 2403.653 4.782
Concurrent Task 10 1007.791 1009.426 1008.763 0.514

@github-actions
Copy link

github-actions bot commented Dec 22, 2022 •
edited

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 107.166 107.167 +0.001 +0.001%
boids 171.210 171.209 -0.001 -0.001%
communication_child_to_parent 91.272 91.274 +0.002 +0.002%
communication_grandchild_with_grandparent 105.621 105.623 +0.002 +0.002%
communication_grandparent_to_grandchild 101.465 101.464 -0.001 -0.001%
communication_parent_to_child 88.363 88.362 -0.001 -0.001%
contexts 108.143 108.140 -0.003 -0.003%
counter 86.298 86.301 +0.003 +0.003%
counter_functional 86.612 86.614 +0.002 +0.002%
dyn_create_destroy_apps 89.147 89.149 +0.002 +0.002%
file_upload 100.773 100.770 -0.004 -0.004%
function_memory_game 165.310 165.301 -0.009 -0.005%
function_router 349.535 349.791 +0.256 +0.073%
function_todomvc 160.228 160.228 0 0.000%
futures 225.326 225.326 0 0.000%
game_of_life 107.060 107.061 +0.001 +0.001%
immutable 182.004 182.000 -0.004 -0.002%
inner_html 82.716 82.717 +0.001 +0.001%
js_callback 112.291 112.295 +0.004 +0.003%
keyed_list 196.718 196.720 +0.002 +0.001%
mount_point 85.869 85.868 -0.001 -0.001%
nested_list 113.861 113.864 +0.003 +0.003%
node_refs 93.772 93.773 +0.001 +0.001%
password_strength 1549.960 1549.961 +0.001 +0.000%
portals 97.058 97.057 -0.001 -0.001%
router 319.608 319.879 +0.271 +0.085%
simple_ssr 151.867 151.866 -0.001 -0.001%
ssr_router 394.013 394.236 +0.224 +0.057%
suspense 109.425 109.425 0 0.000%
timer 89.194 89.192 -0.002 -0.002%
todomvc 141.662 141.662 0 0.000%
two_apps 86.922 86.923 +0.001 +0.001%
web_worker_fib 152.195 152.189 -0.006 -0.004%
webgl 85.445 85.447 +0.002 +0.002%

✅ None of the examples has changed their size significantly.

hamza1311
hamza1311 approved these changes Dec 22, 2022
View reviewed changes
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good

WorldSEnder
WorldSEnder approved these changes Dec 22, 2022
View reviewed changes
Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Very nice and concise solution, looks good to me!

@WorldSEnder WorldSEnder changed the title Fix for Issue 2911 Only handle "normal" clicks on <Link>s Dec 22, 2022
@WorldSEnder WorldSEnder merged commit 1542e2b into yewstack:master Dec 22, 2022
@WorldSEnder WorldSEnder added the A-yew-router Area: The yew-router crate label Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WorldSEnder WorldSEnder WorldSEnder approved these changes

@hamza1311 hamza1311 hamza1311 approved these changes

Assignees
No one assigned
Labels
A-yew-router Area: The yew-router crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Link component should NOT prevent_default when command + clicked
3 participants
@kaisalmon @WorldSEnder @hamza1311