Add tooltips to buttons and icons for better UX - #122
Conversation
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is nice. Please will you add it to more components and pages, though.
|
Thanks for the feedback! |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a new Tooltip component (wrapping react-tooltip), test coverage and docs, and applies tooltip wrappers across the UI (NavBar, pages, image processor, credits). Adds react-tooltip dependency and a ResizeObserver mock for tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/WasmImageProcessor.jsx (1)
183-183: Title attribute on hidden input is ineffective.The
titleattribute on ahiddeninput provides no accessibility benefit since the element is not visible or focusable by users. Consider removing it.- <input ref={inputRef} id={inputId} type="file" accept="image/*" hidden title="Choose an image file to upload" onChange={handleSelect} /> + <input ref={inputRef} id={inputId} type="file" accept="image/*" hidden onChange={handleSelect} />src/pages/Home/index.jsx (1)
17-24: Consider whether titles on non-interactive cards add value.These
GlassCardcomponents are informational (not interactive), and already have clear<h3>headings describing their purpose. Thetitleattributes ("Performance feature", "Integration feature") will show generic tooltips on hover that don't provide additional information beyond the visible headings.For accessibility,
titleis most useful on interactive elements where it clarifies an action. For purely informational cards, these titles may add tooltip noise without benefit.src/components/NavBar.jsx (1)
39-39: Minor inconsistency in tooltip text.Line 12 defines the Home link tooltip as
"Go to the home page"(with "the"), but the logo link here uses"Go to home page"(without "the"). Consider aligning for consistency.- <Link to="/" title="Go to home page">Img2Num</Link> + <Link to="/" title="Go to the home page">Img2Num</Link>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/NavBar.jsx(2 hunks)src/components/WasmImageProcessor.jsx(4 hunks)src/data/contributor-credits.json(1 hunks)src/pages/Home/index.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Home/index.jsx (1)
src/components/GlassCard.jsx (1)
GlassCard(4-8)
🔇 Additional comments (7)
src/components/WasmImageProcessor.jsx (3)
138-140: Good tooltip additions for the empty state UI.The titles on the
Uploadicon and "Choose File" span provide helpful context for users.
155-163: LGTM!Clear tooltip explaining the button action.
172-180: LGTM!The title on the GlassCard drop zone provides helpful context for the drag-and-drop interaction.
src/data/contributor-credits.json (1)
23-29: LGTM!New contributor entry follows the established schema and JSON structure is valid.
src/components/NavBar.jsx (3)
12-15: Good approach to centralize tooltip definitions with links.Clean pattern that keeps tooltip text co-located with link configuration.
42-42: LGTM!Good addition of
titlealongside the existingaria-label— the aria-label serves screen readers while the title provides a visual tooltip for sighted users.
23-28: LGTM!Tooltips are properly applied to both external and internal links, with a helpful additional tooltip on the external link icon indicating it opens in a new tab.
…its and home pages
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/Home/index.jsx (1)
18-18: Reconsider usingtitleattributes on large container elements.Adding
titleattributes to GlassCard components creates native browser tooltips on entire card containers. This UX pattern is unusual because:
- Native title tooltips appear after a delay and only show on hover (not useful for touch/mobile)
- Tooltips on large containers can be confusing—users don't expect cards to have tooltips
- The PR objective mentions "tooltips to buttons and icons," but these are feature cards, not interactive controls
If these titles are meant to improve accessibility or provide metadata, consider alternative approaches like
aria-labelor visually hidden text. If tooltips are truly needed, they should target specific interactive elements within the cards.💡 Consider removing the title attributes:
- <GlassCard title="Performance feature"> + <GlassCard> <h3>⚡ Fast & Lightweight</h3> <p>Compiled C++ runs in your browser via WebAssembly with near-native speed.</p> </GlassCard> - <GlassCard title="Integration feature"> + <GlassCard> <h3>🛠️ Easy to Integrate</h3> <p>Minimal dependencies, works with any project or workflow.</p> </GlassCard>Also applies to: 22-22
src/pages/Credits/ContributorsCreditsCard.jsx (1)
24-24: Consider using consistent tooltip text for both links.Both the avatar and login links navigate to the same GitHub profile URL but use different verbs ("Open" vs "Visit"). Using consistent wording improves clarity.
💡 Apply this diff for consistency:
- <a href={c.html_url} target="_blank" rel="noopener noreferrer" title={`Open ${c.login}'s Github profile`}> + <a href={c.html_url} target="_blank" rel="noopener noreferrer" title={`Visit ${c.login}'s GitHub profile`}> <FallbackImage src={c.avatar_url} fallback={<User color={'var(--color-text-light)'} />} alt={c.login} width="28" height="28" style={{ borderRadius: '50%' }} /> </a>- <a href={c.html_url} target="_blank" rel="noopener noreferrer" title={`Visit ${c.login}'s Github profile`}> + <a href={c.html_url} target="_blank" rel="noopener noreferrer" title={`Visit ${c.login}'s GitHub profile`}> {c.login} </a>Note: Also corrected "Github" → "GitHub" (proper capitalization).
Also applies to: 36-36
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pages/Credits/ContributorsCreditsCard.jsx(2 hunks)src/pages/Credits/DependencyCreditsCard.jsx(1 hunks)src/pages/Credits/StaticCreditsCard.jsx(3 hunks)src/pages/Home/index.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Home/index.jsx (2)
src/components/Hero.jsx (1)
Hero(4-9)src/components/GlassCard.jsx (1)
GlassCard(4-8)
🪛 Biome (2.1.2)
src/pages/Credits/StaticCreditsCard.jsx
[error] 13-13: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 22-22: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 27-27: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 38-38: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 42-42: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 46-46: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🔇 Additional comments (1)
src/pages/Credits/DependencyCreditsCard.jsx (1)
66-66: LGTM! Well-implemented tooltip with proper security attributes.The title attribute provides clear context, and the link correctly includes
rel="noopener noreferrer"for security.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/About/Author.jsx (1)
13-21: LGTM! Tooltips successfully added.The
titleattributes provide clear, descriptive tooltips that enhance UX by clarifying link destinations. The implementation is straightforward and meets the PR objectives.Optional enhancement for future consideration:
While
titleattributes work well for mouse users, they have some accessibility limitations:
- Not available to touch screen users
- Inconsistent behavior with screen readers
- May not appear on keyboard focus in all browsers
For stronger accessibility, consider using
aria-labelor a dedicated tooltip library (like react-tooltip) in future iterations. However, since the original issue #103 explicitly acceptedtitleattributes as a valid approach, the current implementation is perfectly acceptable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/About/CTA.jsx (1)
9-9: Tooltip doesn't add meaningful value.The
titleattribute repeats information already conveyed by the visible link text ("Launch Img2Num"). For accessibility, tooltips implemented viatitleattributes have limited support—they're not consistently announced by screen readers, unavailable on touch screens, and not fully keyboard-accessible.Consider either removing the
titleattribute or providing genuinely additional context. For example, if you want to clarify what "Img2Num" means, you might use:title="Go to homepage and start converting images to color-by-number". Otherwise, omit the tooltip when the visible text is already clear.Note: Based on the enriched summary, this pattern appears across multiple components in this PR. You may want to review all added tooltips to ensure they provide unique, helpful information rather than duplicating visible text.
|
Thanks for the feedback! I’ve added tooltips to more components and pages, and also fixed the external links by adding rel="noopener noreferrer". Please let me know if anything else needs changes. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Ryan-Millard. * #122 (comment) The following files were modified: * `src/components/NavBar.jsx` * `src/pages/Credits/ContributorsCreditsCard.jsx` * `src/pages/Credits/DependencyCreditsCard.jsx` * `src/pages/Credits/StaticCreditsCard.jsx`
I'll be reviewing it soon. Thank you for your contribution! |
|
Thanks! Happy to make any changes if needed. |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is very good and I'm happy with it!
There are just a few minor tweaks I'd like you to implement, which are addressed on individual files.
Additionally, although it isn't part of #103), please will you consider adding react-tooltip to this PR (as CodeRabbit mentioned) because it will handle cases we probably won't even think of (see below).
I strongly recommend creating a reusable component for this as well as it will allow us to have a way to trigger errors when someone forgets to add something (like a title) as a prop. This will greatly help people (such as myself) who are more focused on creating a feature than a user-friendly interface at times.
That'll also beef up your contributions and the overall impact you have on this project - which will look nice on a CV when this repository grows.
|
Thank you again for the great work you've done so far, @sarvesh0415! |
|
Hi Ryan 👋 Please let me know if anything else needs tweaking. Thanks again! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/pages/About/Author.jsx (1)
25-29: Address the past review comment regarding spacing.The previous review comment from Ryan-Millard requested a specific format that removes the
{' '}fragments and consolidates the line. The current implementation still includes these fragments and doesn't match the suggested format.Additionally, there's an inconsistency in the tooltip approach: external links (GitHub, LinkedIn) use native
titleattributes, while this internal Link uses the Tooltip component. Consider standardizing on one approach—either usetitleattributes everywhere or use the Tooltip component consistently.Apply the suggested format from the past review:
- While I led this project, I had some help from others—see the{' '} - <Tooltip content="View project credits"> - <Link to="/credits">Credits page</Link> - </Tooltip>{' '} - for details. + While I led this project, I had some help from others—see the <Link to="/credits" title="View project credits">Credits page</Link> for details.This approach:
- Matches the format requested in the past review
- Uses native
titleattribute consistent with the external links above- Simplifies the code by removing the Tooltip wrapper and
{' '}fragmentsBased on past review comments.
🧹 Nitpick comments (1)
src/components/Tooltip.jsx (1)
13-15: Consider potential styling and accessibility implications of the span wrapper.The span wrapper is a common pattern for react-tooltip, but it may affect:
- Layout/styling if children are inline elements or have specific display requirements
- Accessibility tree structure
- Event bubbling or propagation
This is likely fine for most use cases, but be aware of these potential edge cases when wrapping complex components.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/components/Tooltip.jsx(1 hunks)src/pages/About/Author.jsx(2 hunks)src/pages/About/CTA.jsx(1 hunks)src/pages/Home/index.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/About/CTA.jsx
- src/pages/Home/index.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/About/Author.jsx (1)
src/components/Tooltip.jsx (1)
Tooltip(4-20)
🔇 Additional comments (4)
src/pages/About/Author.jsx (2)
4-4: LGTM!The Tooltip import is correctly placed and follows the established import pattern in the file.
16-21: Good accessibility addition with title attributes.The title attributes and
rel="noopener noreferrer"on external links improve both accessibility and security.src/components/Tooltip.jsx (2)
1-2: LGTM!Imports are correctly structured and the react-tooltip library is properly imported.
22-26: LGTM!PropTypes are correctly defined with appropriate types and required flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Tooltip.jsx (2)
22-26: Consider guarding against empty content strings.The
contentprop is marked as required, but PropTypes allows empty strings. Ifcontent=""is passed, the tooltip will render but display nothing, which could confuse users.🔎 Add a custom validator or runtime check:
Option 1: Custom PropTypes validator
Tooltip.propTypes = { - content: PropTypes.string.isRequired, + content: function(props, propName, componentName) { + const value = props[propName]; + if (!value || typeof value !== 'string') { + return new Error( + `Invalid prop \`${propName}\` supplied to \`${componentName}\`. Expected a non-empty string.` + ); + } + }, children: PropTypes.node.isRequired, id: PropTypes.string, };Option 2: Runtime check (development only)
export default function Tooltip({ content, children, id }) { + if (process.env.NODE_ENV !== 'production' && !content?.trim()) { + console.warn('Tooltip: content prop should not be empty'); + } const reactId = useId(); const tooltipId = id || `tooltip-${reactId}`;
1-20: Previous feedback addressed; consider performance optimization.The component now correctly uses
useId()for stable ID generation, addressing the previous issue withMath.random(). The runtime error throwing has also been removed. Great improvements!However, creating a new
ReactTooltipinstance for each tooltip (line 17) is not optimal. The recommended pattern with react-tooltip v5 is to render a single<ReactTooltip />at the app or page level, then reference it viadata-tooltip-idfrom multiple elements. This reduces overhead when many tooltips exist.🔎 Optional: Refactor for better performance with a global ReactTooltip
App-level approach (recommended):
- Remove
<ReactTooltip id={tooltipId} />from this component:export default function Tooltip({ content, children, id }) { const reactId = useId(); const tooltipId = id || `tooltip-${reactId}`; return ( - <> <span data-tooltip-id={tooltipId} data-tooltip-content={content}> {children} </span> - - <ReactTooltip id={tooltipId} /> - </> ); }
- Then add a single
<ReactTooltip />in your root layout or App component (without anidprop—it will handle all tooltips):import { ReactTooltip } from 'react-tooltip'; function App() { return ( <div> {/* your app content */} <ReactTooltip /> </div> ); }This way, one tooltip instance serves all tooltip triggers across the app.
Minor: Remove unnecessary blank lines.
Lines 6 and 10 contain extra blank lines that can be removed for cleaner formatting.
Verify react-tooltip v5 API usage.
Please confirm the current pattern (one
ReactTooltipper tooltip) works as expected in react-tooltip v5.30.0, especially for accessibility (keyboard focus, screen readers) and when multiple tooltips are rendered on the same page.#!/bin/bash # Verify react-tooltip v5 usage pattern and check for multiple ReactTooltip instances # Find all usages of the Tooltip component rg -n --type=tsx --type=jsx -C3 '<Tooltip' # Find if a global ReactTooltip is already rendered elsewhere rg -n --type=tsx --type=jsx '<ReactTooltip'
|
I’ve addressed the requested changes by adding a keyboard accessibility test for the Tooltip component. Please let me know if anything else is needed. |
Thank you very much! It looks great. I'll review it as soon as I can - hopefully before tomorrow. |
…nt, now function for clarity
- Tooltip now attaches to focusable children correctly - NavBar external link tooltips fixed - Updated Tooltip tests for keyboard and hover behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/docs/reference/react/components/Tooltip/index.md (1)
31-37: Add blank line before props table for proper markdown formatting.Markdown best practices require blank lines around tables for consistent rendering across parsers.
🔎 Proposed fix
## Props + | Prop | Type | Required | Default | Notes |src/components/NavBar.jsx (1)
57-63: Consider aligning tooltip and aria-label text for consistency.The tooltip says "Toggle navigation menu" while the aria-label says "Toggle menu". While both are clear, using identical text ensures consistency for users who rely on different assistive technologies.
🔎 Proposed alignment
- <Tooltip content="Toggle navigation menu"> + <Tooltip content="Toggle menu"> <button className={styles.hamburger} onClick={() => setIsOpen(!isOpen)} aria-label="Toggle menu">Or alternatively, expand the aria-label to match the tooltip.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/docs/guidelines/_category_.json(1 hunks)docs/docs/project-scripts/_category_.json(1 hunks)docs/docs/reference/react/components/Tooltip/index.md(1 hunks)docs/docs/reference/react/components/Tooltip/tests.md(1 hunks)docs/docs/reference/react/components/_category_.json(1 hunks)src/components/NavBar.jsx(1 hunks)src/components/Tooltip.jsx(1 hunks)src/components/Tooltip.test.jsx(1 hunks)src/components/WasmImageProcessor.jsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Tooltip.jsx
- src/components/Tooltip.test.jsx
- src/components/WasmImageProcessor.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/NavBar.jsx (2)
src/pages/Editor/index.jsx (1)
useLocation(7-7)src/components/Tooltip.jsx (1)
Tooltip(5-44)
🪛 Biome (2.1.2)
docs/docs/reference/react/components/_category_.json
[error] 9-10: Expected a property but instead found '}'.
Expected a property here.
(parse)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/react/components/Tooltip/index.md
32-32: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
37-37: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔇 Additional comments (8)
docs/docs/guidelines/_category_.json (1)
2-2: LGTM! Emoji removal for consistency.The label cleanup improves consistency across documentation categories.
docs/docs/project-scripts/_category_.json (1)
2-2: LGTM! Consistent label cleanup.Aligns with the emoji removal pattern applied to other documentation categories.
docs/docs/reference/react/components/Tooltip/index.md (1)
1-127: Excellent documentation! Past feedback addressed.The documentation now includes comprehensive coverage of:
- Props table with types, requirements, defaults, and detailed usage notes
- Implementation details explaining the wrapper approach, portal rendering, and ID generation
- Multiple practical examples for different use cases
- Accessibility guidance with good/bad pattern examples
- Testing tips with code samples
This thoroughly addresses the previous review feedback requesting props explanation and implementation details.
docs/docs/reference/react/components/Tooltip/tests.md (1)
1-44: Comprehensive test documentation! Past feedback addressed.The documentation now provides:
- Clear overview of the test suite's purpose and scope
- Detailed explanations of individual test cases and their assertions
- Technical depth on common issues: portal timing, animations, jsdom environment, event ordering, double focus, strict mode warnings
- Practical code examples for handling test flakiness
- Clear testing recommendations for maintainability
This thoroughly addresses the previous review feedback requesting an overview and more technical depth.
src/components/NavBar.jsx (4)
13-16: LGTM! Clear and descriptive tooltip text.The tooltip messages are concise and effectively communicate each link's purpose.
25-35: Excellent! Single tooltip approach addresses past feedback.The external link now uses a single tooltip wrapping the entire anchor with a combined message that includes both the link purpose and the new-tab behavior. This resolves the previous concern about dual tooltips causing confusion.
37-41: LGTM! Clean internal link tooltip integration.The Tooltip correctly wraps the Link component, allowing the tooltip attributes to attach directly to the focusable element.
52-54: LGTM! Clear logo tooltip.The tooltip provides helpful context for the logo's navigation behavior.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
Thank you for the great work you put in!
|
Thank you so much for the review and for merging the PR! |
|
Hi @Ryan-Millard, I noticed a small UX issue on the website: the tooltip for the “Fast & Lightweight” section appears to be clipped / partially hidden (possibly due to an overflow or z-index constraint). Just wanted to flag this in case it helps — happy to look into it later if needed. |
Hi. Thanks for spotting that! Please will you open an issue for this. The correct issue template is the bug one. I think the issue is about z-index, it may need to be applied to every tooltip. |
|
Sure, I’ll open a bug issue for this shortly. Thanks! |
Thank you! Have a good day! |
2 participants
Fixes #103
What’s changed
Why
Scope
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.