-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
feat: add image zoom to improve image viewing experience #20763
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
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @reidliu41, 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!
This pull request introduces a significant user experience improvement to the documentation by enabling click-to-zoom functionality for images. This feature utilizes the medium-zoom
library to provide a smooth and interactive way for users to view detailed diagrams and screenshots, enhancing the overall readability and utility of the documentation.
Highlights
- Documentation Enhancement: Implemented a new image zoom feature for documentation images, allowing users to click and enlarge them.
- User Experience: Improves the clarity and interactivity for viewing diagrams and screenshots within the documentation, making complex visuals easier to understand.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a nice image zoom feature to the documentation using the medium-zoom
library. The implementation is clean and simple. I have two suggestions to improve it:
- Security: Add Subresource Integrity (SRI) to the external
medium-zoom
script inmkdocs.yaml
to protect against potential CDN compromises. - Robustness: In
image_zoom.js
, add a check to ensure themediumZoom
function is available before calling it. This will prevent JavaScript errors if the external library fails to load.
These changes will make the new feature more secure and resilient.
mkdocs.yaml
Outdated
@@ -128,6 +128,8 @@ extra_javascript: | |||
- https://cdn.mathjax.org/mathjax/latest/MathJax.js?config=TeX-AMS_HTML | |||
- mkdocs/javascript/edit_and_feedback.js | |||
- mkdocs/javascript/slack_and_forum.js | |||
- https://cdn.jsdelivr.net/npm/[email protected]/dist/medium-zoom.min.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading JavaScript from a third-party CDN without Subresource Integrity (SRI) poses a security risk. If the CDN were to be compromised, malicious code could be injected into the documentation pages, leading to Cross-Site Scripting (XSS) vulnerabilities.
To mitigate this, you should add an integrity
hash and crossorigin
attribute for the medium-zoom
script. This ensures that the browser will only execute the script if it matches the expected cryptographic hash.
- url: https://cdn.jsdelivr.net/npm/[email protected]/dist/medium-zoom.min.js
integrity: sha256-ZgMyR+SN2PSZ9X/dG2wUeC2e3iT3eT5s21iL3g2QyY0=
crossorigin: anonymous
docs/mkdocs/javascript/image_zoom.js
Outdated
document$.subscribe(() => { | ||
mediumZoom('article img:not(.no-zoom)', { | ||
background: 'rgba(0, 0, 0, 0.8)', | ||
margin: 24, | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation assumes mediumZoom
is always available. If the external script from the CDN fails to load due to network issues or CDN downtime, this script will throw a ReferenceError
, which could break other JavaScript functionality on the page.
To make the code more robust, it's best to check if mediumZoom
is defined as a function before calling it.
document$.subscribe(() => { | |
mediumZoom('article img:not(.no-zoom)', { | |
background: 'rgba(0, 0, 0, 0.8)', | |
margin: 24, | |
}); | |
}); | |
document$.subscribe(() => { | |
if (typeof mediumZoom === 'function') { | |
mediumZoom('article img:not(.no-zoom)', { | |
background: 'rgba(0, 0, 0, 0.8)', | |
margin: 24, | |
}); | |
} | |
}); |
@hmellor can you review this? |
Could we use a mkdocs plugin instead of something custom we have to maintain? https://squidfunk.github.io/mkdocs-material/reference/images/#lightbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking until we decide which solution to go with
03f1863
to
90c392f
Compare
updated with |
90c392f
to
47e3834
Compare
hi, May I know any concerns about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you merge from main? There is a different fix for the gguf
issue
Signed-off-by: reidliu41 <[email protected]>
47e3834
to
1bc38aa
Compare
hi @DarkLight1337 can you help to merge if you are online? thanks |
just tested, looks cool.. |
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.Purpose
This enables click-to-zoom on images in documentation using medium-zoom.
It helps users view images more clearly and interactively.
Test Plan
Test Result
(Optional) Documentation Update