-
Notifications
You must be signed in to change notification settings - Fork 1
Allow tooltips to wrap if the content length exceeds the max-width #24
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
base: develop
Are you sure you want to change the base?
Conversation
Which can be the case for translations
WalkthroughTooltip styling is updated in both the minified and unminified accessibility new window warnings JavaScript files to enable text wrapping by changing the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @pattonwebz, 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 addresses an issue where tooltip content would not wrap, leading to truncated or unreadable text, particularly with internationalized strings. By adjusting a single CSS property, tooltips will now gracefully wrap their content, ensuring all information is visible and enhancing the overall user experience. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
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 correctly enables text wrapping in tooltips by changing the whiteSpace CSS property from nowrap to normal. This is a good improvement, especially for translated strings that can be longer. The change is implemented correctly in both the source and minified JavaScript files. I've added one suggestion to improve maintainability by separating styles into a CSS file, which you can consider.
| boxShadow: "0px 4px 6px rgba(0,0,0,0.1)", | ||
| maxWidth: "200px", | ||
| whiteSpace: "nowrap" | ||
| whiteSpace: "normal", |
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.
While this change correctly enables text wrapping, you could improve maintainability by moving all static tooltip styles to your CSS file. This separates presentation (CSS) from behavior (JS).
For example, you could add the following to assets/css/accessibility-new-window-warnings.css:
.anww-tooltip {
position: absolute;
background: white;
color: #1e1e1e;
font-size: 16px;
border: 1px solid black;
padding: 5px 10px;
z-index: 9999;
display: none;
pointer-events: auto;
box-shadow: 0px 4px 6px rgba(0,0,0,0.1);
max-width: 200px;
white-space: normal;
}Then, the initializeTooltip function in this file could be simplified by removing the Object.assign call that sets these styles inline. This is an optional suggestion for future improvement.
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.
Pull Request Overview
This PR enables tooltip text wrapping when content exceeds the 200px max-width, which is particularly beneficial for long translated strings that previously would overflow.
- Changed
whiteSpaceCSS property from"nowrap"to"normal"to allow text wrapping - Updated both source and minified JavaScript files consistently
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| assets/js/accessibility-new-window-warnings.js | Changed tooltip styling to allow text wrapping by setting whiteSpace to "normal" |
| assets/js/accessibility-new-window-warnings-min.js | Minified version updated to match the source file changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
assets/js/accessibility-new-window-warnings.js (1)
26-26: Consider whether 200px max-width is sufficient for all translations.While the wrapping fix addresses the overflow issue, you may want to verify that 200px provides adequate space for typical translated strings. If user feedback indicates that tooltips are excessively tall or difficult to read in certain languages, you could consider increasing this value (e.g., to 250px or 300px) in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
assets/js/accessibility-new-window-warnings-min.js(1 hunks)assets/js/accessibility-new-window-warnings.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Upload results
🔇 Additional comments (3)
assets/js/accessibility-new-window-warnings.js (2)
27-27: LGTM! Change correctly enables text wrapping for translated tooltips.The change from
whiteSpace: "nowrap"towhiteSpace: "normal"directly addresses issue #23, allowing tooltip text to wrap within the 200px max-width constraint. This prevents translated strings from overflowing the tooltip background.
155-179: Positioning logic correctly handles wrapped tooltip dimensions.The
showTooltipfunction measuresoffsetWidthandoffsetHeightafter setting the tooltip todisplay: "block"(lines 161-162), which means it will correctly account for the increased height when text wraps. The overflow adjustment logic (lines 168-175) will properly reposition wrapped tooltips that would otherwise extend beyond viewport edges.assets/js/accessibility-new-window-warnings-min.js (1)
1-1: The minified file has been properly regenerated from source.The source file contains
whiteSpace: "normal"on line 27, and this change is correctly reflected in the minified version. The repository has build infrastructure properly configured (CodeKit3 and Prepros) for automatic minification, confirming that the minified file was generated from the source rather than manually edited. The change correctly addresses the tooltip wrapping issue for translated strings.
Allows warping of tooltips when the space isn't enough to fix the text (such as is often the case with translated strings)
Fixes: #23
Summary by CodeRabbit