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

Feature request: Improve scaling API to simplify zoom implementation in embedders #18076 #18098

Closed
wants to merge 1 commit into from
Closed

Feature request: Improve scaling API to simplify zoom implementation in embedders #18076 #18098

wants to merge 1 commit into from

Conversation

harshsbhat
Copy link

…in embedders #18076

This pull request introduces a significant enhancement to the scaling API by adding a center: [x, y] parameter to the increaseScale and decreaseScale functions. This new parameter allows for customization of the zoom center, eliminating the need for manual adjustment of scroll positions after zoom operations. Previously, zooming would always scroll to _location.top and _location.left, requiring additional calculations to ensure the correct zoom center. With this enhancement, developers can specify the exact zoom center, simplifying the implementation of zoom features and ensuring more predictable behavior across various positions and zoom levels within the viewer.

  1. Added center: [x, y] parameter to increaseScale and decreaseScale functions.
  2. Have NOT implemented the updated scale function.

Comment on lines +2146 to +2147
// Adjust scroll position based on the provided center coordinates
// (Implementation of scroll adjustment omitted for brevity)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a maintainer of pdf.js, but if I were I would really dislike somebody just copy-pasting the issue text as a prompt for some LLM, copying the resulting code, and opening a pull request without neither reading nor understanding the changes they are submitting.

@Snuffleupagus
Copy link
Collaborator

Sorry, but this unfortunately does not appear to be a working patch.

First of all, there's a bunch of unrelated code changes which are also going to fail linting.
Secondly, the center parameter doesn't actually seem to do anything which renders this patch moot.
Finally, at lot of the comments seems redundant given the actual code. (And I don't think that we need updateScale since that's mostly code duplication.)

@harshsbhat harshsbhat deleted the feature/zoom-enhancements branch May 15, 2024 15:22
@harshsbhat
Copy link
Author

I understand both of you @Snuffleupagus and @nicolo-ribaudo . I was just trying to work on some real repo for the first time. Of course, in the process, I took help from ChatGPT for some parts of the code. Thank you, guys, for your feedback, I will try to find some beginner-friendly issues on this repo. Sorry for wasting your time.

@harshsbhat
Copy link
Author

@Snuffleupagus So as I mentioned in the pull request I decided to not add the updateScale function in the app.js. But forgot to delete it from the pdf_viewer.js. @nicolo-ribaudo Can you add a little more information on what needs to be added in my code?

@Snuffleupagus
Copy link
Collaborator

Unfortunately I don't believe that the referenced issue is a good beginner bug, even more so if you're trying to do development with the "help" of ChatGPT.

@harshsbhat
Copy link
Author

harshsbhat commented May 15, 2024

Okay, thanks for that I guess @Snuffleupagus . I guess I should improve my development skills before trying to make my first open-source contribution

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.

3 participants