Change lock image to zoom to 100 percent and option to start zoomed#1582
Conversation
LiliaDoe
commented
Mar 21, 2025
LiliaDoe
commented
- Adds option to make locked image not zoom using zoom factor
- Adds option to make locked images start zoomed
- Should resolve Few suggestions #1574 at last
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @LiliaDoe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request by LiliaDoe addresses issue #1574 by adding options to control the zoom behavior of locked images. Specifically, it introduces settings to either use the zoom factor for locked images or to start locked images zoomed in by default. The changes involve adding new options to the settings page, updating the default settings, and modifying the zoom logic in the hoverzoom.js file.
Highlights
- New Options: Adds two new options to the settings page: 'Use zoom factor for lock image' and 'Locked images start zoomed in'.
- Zoom Logic Modification: Modifies the zoom logic in
hoverzoom.jsto incorporate the new options, allowing users to control whether locked images zoom to 100% or use the zoom factor, and whether they start zoomed in. - Default Settings: Updates the default settings in
common.jsto enable the 'Use zoom factor for lock image' option and disable the 'Locked images start zoomed in' option by default.
Changelog
Click here to see the changelog
- _locales/en/messages.json
- Added new message keys and descriptions for the new options:
optLockImageZoomFactorEnabled,optLockImageZoomFactorEnabledTooltip,optLockImageZoomDefaultEnabled, andoptLockImageZoomDefaultEnabledTooltip.
- Added new message keys and descriptions for the new options:
- html/options.html
- Added HTML elements (checkboxes) for the new options:
chkLockImageZoomFactorEnabledandchkLockImageZoomDefaultEnabled.
- Added HTML elements (checkboxes) for the new options:
- js/common.js
- Added new default settings for
lockImageZoomFactorEnabled(true) andlockImageZoomDefaultEnabled(false).
- Added new default settings for
- js/hoverzoom.js
- Modified the
lockImagefunction to incorporate the new options when determining the zoom factor for locked images. The logic now checks thelockImageZoomDefaultEnabledoption to determine whether to start zoomed in, and thelockImageZoomFactorEnabledoption to determine whether to use the zoom factor or 100% zoom.
- Modified the
- js/options.js
- Modified
saveOptionsfunction to save the state of the new checkboxes (chkLockImageZoomFactorEnabledandchkLockImageZoomDefaultEnabled) to the options object. - Modified
restoreOptionsfunction to restore the state of the new checkboxes based on the options object.
- Modified
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
The zoom, a fickle friend,
Sometimes near, sometimes end.
With options new, we take control,
Of images locked, body and soul.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces new options to control the zoom behavior of locked images, providing users with more flexibility. The code appears well-structured and addresses the issue outlined in the description. However, there are a few areas where improvements can be made to enhance clarity and maintainability.
Summary of Findings
- Clarity of conditional logic: The nested conditional logic in
hoverzoom.jscan be simplified to improve readability and reduce complexity. Consider using a single conditional statement with combined conditions. - Consistency in variable usage: In
hoverzoom.js, the use ofparseInt(options.zoomFactor)is repeated multiple times. It would be more efficient and readable to store this value in a variable and reuse it. - Default value for
lockImageZoomFactorEnabled: The default value forlockImageZoomFactorEnabledincommon.jsis set totrue. Consider whether this is the most intuitive default behavior for users.
Merge Readiness
The pull request is almost ready for merging. Addressing the comments regarding the conditional logic and variable usage in hoverzoom.js would significantly improve the code's readability and maintainability. While the default value for lockImageZoomFactorEnabled is a matter of preference, it's worth considering whether it aligns with the intended user experience. I am unable to approve this pull request, and users should have others review and approve this code before merging. I recommend that the pull request not be merged until the comments are addressed.
| if (options.lockImageZoomDefaultEnabled) | ||
| zoomFactor = (options.lockImageZoomFactorEnabled) ? parseInt(options.zoomFactor) : 1; | ||
| else | ||
| zoomFactor = zoomFactorFit || parseInt(options.zoomFactor); |
There was a problem hiding this comment.
This conditional logic can be simplified. Consider combining the conditions into a single if statement for better readability. For example, you could use zoomFactor = (options.lockImageZoomDefaultEnabled && options.lockImageZoomFactorEnabled) ? parseInt(options.zoomFactor) : zoomFactorFit || parseInt(options.zoomFactor);
zoomFactor = (options.lockImageZoomDefaultEnabled && options.lockImageZoomFactorEnabled) ? parseInt(options.zoomFactor) : zoomFactorFit || parseInt(options.zoomFactor);| if (zoomFactor !== zoomFactorFit) { | ||
| // Makes image fits within screen | ||
| zoomFactor = zoomFactorFit || parseInt(options.zoomFactor); | ||
| } else { | ||
| // Makes image zoom to default or 100% | ||
| zoomFactor = (options.lockImageZoomFactorEnabled) ? parseInt(options.zoomFactor) : 1; | ||
| } |
There was a problem hiding this comment.
Consider simplifying this nested conditional logic. You could use a single if statement with combined conditions, similar to the earlier suggestion. Also, parseInt(options.zoomFactor) is used multiple times; consider storing it in a variable to avoid redundant calculations and improve readability.
const zoomFactorDefault = parseInt(options.zoomFactor);
if (zoomFactor !== zoomFactorFit) {
zoomFactor = zoomFactorFit || zoomFactorDefault; // Makes image fits within screen
} else {
zoomFactor = (options.lockImageZoomFactorEnabled) ? zoomFactorDefault : 1; // Makes image zoom to default or 100%
}…thub.com/LiliaDoe/hoverzoom into Change-lock-image-to-zoom-to-100-percent
|
There was a problem hiding this comment.
Code Review
This pull request introduces new options to control the zoom behavior of locked images, providing users with more flexibility. The changes seem well-structured and address the issue #1574 as intended. However, there are a few areas that could benefit from further refinement to enhance clarity and maintainability.
Summary of Findings
- Clarity of Logic in
lockImageKey: The logic within thelockImageKeyfunction could be simplified to improve readability and reduce potential confusion regarding the zoom behavior. - Inconsistent Naming: The naming of variables such as
zoomFactorDefaultandzoomDefaultEnabledcould be more consistent to improve code clarity. - Missing input validation: The code does not validate the zoom factor input, which could lead to unexpected behavior if the input is invalid.
Merge Readiness
The pull request is almost ready for merging. Addressing the comments regarding the logic in lockImageKey and ensuring consistent naming would significantly improve the code's quality. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. It is recommended that the pull request not be merged until the high severity issues are addressed.
| } | ||
|
|
||
| function lockImageKey(event) { | ||
| const zoomFactorDefault = parseInt(options.zoomFactor); |
There was a problem hiding this comment.
It might be beneficial to add a check to ensure that options.zoomFactor is a valid number before parsing it. This could prevent unexpected behavior if the option is misconfigured.
const zoomFactorValue = options.zoomFactor;
const zoomFactorDefault = isNaN(parseInt(zoomFactorValue)) ? 1 : parseInt(zoomFactorValue);|
|
||
| let width = imgFullSize.width() || imgFullSize[0].width; | ||
| zoomFactorFit = width / srcDetails.naturalWidth; | ||
| zoomFactor = (zoomDefaultEnabled && useZoomFactor) ? zoomFactorDefault : zoomFactorFit || zoomFactorDefault; |
There was a problem hiding this comment.
The logic here could be simplified by extracting the condition zoomDefaultEnabled && useZoomFactor into a variable to improve readability. This also avoids repeating the same condition.
| zoomFactor = (zoomDefaultEnabled && useZoomFactor) ? zoomFactorDefault : zoomFactorFit || zoomFactorDefault; | |
| const shouldZoomByDefault = zoomDefaultEnabled && useZoomFactor; | |
| zoomFactor = shouldZoomByDefault ? zoomFactorDefault : zoomFactorFit || zoomFactorDefault; |
| if (zoomFactor !== zoomFactorFit) { | ||
| zoomFactor = zoomFactorFit || zoomFactorDefault; // Makes image fits within screen | ||
| } else { | ||
| zoomFactor = (useZoomFactor) ? zoomFactorDefault : 1; // Makes image zoom to default or 100% |
There was a problem hiding this comment.
Consider using a more descriptive variable name than useZoomFactor to clearly indicate its purpose. Also, the value 1 could be extracted into a constant with a descriptive name.
const zoomToImageFactor = useZoomFactor ? zoomFactorDefault : 1; // Makes image zoom to default or 100%
zoomFactor = zoomToImageFactor;
2 participants
