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

attempt at smart zoom #6

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

thisiscam
Copy link

#5

Code is pretty ad-hoc but seems to work to a good degree.
I will post progress here.

@nizioleque
Copy link
Owner

Hi, it looks great, but I have a couple notes/suggestions:

  • On Windows, this doesn't work at all, because clicking the middle button triggers scroll. You need to always call preventDefault() in your event listener (inside the if).
  • I'm not sure what to do with the default behavior of middle click on links (opening a link in new tab). We can either leave it as it is (it will be possible to open links in a new tab, but zooming on them will also trigger them) or we can disable this default behavior. Or we can disable smart zoom if a link is being clicked - this is probably the best option.
  • For the two reasons above (disabling other browser features), we definitely shouldn't disable this default behavior for all users, so we should probably add an option in the settings to disable the smart zoom.
  • I don't like the fact that on small elements it zooms in very close. We should probably have some maximum scale, or look at the element's size and zoom in on its parent (or the parent's parent, ...) instead if it's too small. I think the second option makes more sense.

Other than that, it's great! Thank you for the contribution. Please let me know if you can fix any of the issues I mentioned. If not, I'll try to do this, but it will probably take me a long time.

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.

2 participants