[WEB-4041] 14.6.8 - pricing card title tooltips, support for multiple tooltip orientations #507
Conversation
WalkthroughThe pull request introduces several updates across multiple files in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/core/Pricing/data.tsx (2)
230-239: LGTM! Consider adding an aria-label for accessibility.The addition of the tooltip for the "Channels" entry is informative and well-implemented. The content is concise and the link to further documentation is appropriate.
To improve accessibility, consider adding an
aria-labelto the link:<a href="/docs/channels" className="ui-link"> + aria-label="Learn more about Ably channels" Find out more. </a>
268-276: LGTM! Consider adding an aria-label for accessibility.The addition of the tooltip for the "Connections" entry is well-implemented and consistent with the "Channels" entry. The content is informative and the link to further documentation is appropriate.
To improve accessibility, consider adding an
aria-labelto the link:<a href="/docs/connect" className="ui-link"> + aria-label="Learn more about Ably connections" Find out more. </a>src/core/Pricing/PricingCards.tsx (2)
109-124: LGTM: Tooltip integration looks good.The new structure for rendering the title with an optional tooltip is well-implemented. The conditional rendering and proper configuration of the
Tooltipcomponent are commendable.Consider adding an
aria-labelto the tooltip container for improved accessibility:<div className="flex items-center mb-12"> + <div aria-label={`${title.content} information`}> <p className={`${title.className ?? ""} ${t(title.color ?? "text-neutral-000")}`} > {title.content} </p> {title.tooltip ? ( <Tooltip className="-mt-4" theme={theme} interactive={typeof title.tooltip !== "string"} > {title.tooltip} </Tooltip> ) : null} + </div> </div>
Line range hint
1-124: Overall implementation looks solid with room for minor optimization.The addition of tooltips to the pricing card titles is well-implemented and aligns with the PR objectives. The changes are localized and maintain consistency with the existing code structure.
Consider memoizing the tooltip content to optimize performance, especially if the content is complex or the component re-renders frequently:
const memoizedTooltip = React.useMemo(() => ( title.tooltip ? ( <Tooltip className="-mt-4" theme={theme} interactive={typeof title.tooltip !== "string"} > {title.tooltip} </Tooltip> ) : null ), [title.tooltip, theme]); // Then use memoizedTooltip in your JSXThis optimization can help reduce unnecessary re-renders of the tooltip content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- package.json (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (2 hunks)
- src/core/Pricing/types.ts (1 hunks)
- src/core/Tooltip.tsx (5 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (7)
src/core/Pricing/types.ts (1)
8-8: LGTM: Addition of tooltip property to PricingDataHeaderThe new
tooltipproperty is a good addition to thePricingDataHeadertype. It allows for more flexibility in displaying additional information for pricing card headers. The use ofstring | ReactNodeas the type is appropriate, allowing for both simple text tooltips and more complex React components.A few points to consider:
- This change aligns well with the PR objectives of adding tooltip functionality to pricing card titles.
- The optional nature of the property (
tooltip?) ensures backward compatibility with existing code usingPricingDataHeader.- The flexibility of using either
stringorReactNodeprovides good extensibility for future use cases.To ensure this change is properly utilized, let's verify its usage in the codebase:
✅ Verification successful
Verified:
tooltipproperty is used correctlyThe
tooltipproperty inPricingDataHeaderis consistently utilized as aReactNodeacross the codebase, aligning with its type definitiontooltip?: string | ReactNode. No type inconsistencies or issues were found.
tooltipis assigned JSX elements insrc/core/Pricing/data.tsx, ensuring flexibility for complex React components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new tooltip property in PricingDataHeader # Test 1: Search for PricingDataHeader usage with tooltip echo "Searching for PricingDataHeader usage with tooltip:" rg --type typescript 'PricingDataHeader.*tooltip' # Test 2: Search for any hardcoded tooltip strings in pricing-related files echo "Searching for hardcoded tooltip strings in pricing-related files:" rg --type typescript 'tooltip:' src/core/PricingLength of output: 424
Script:
#!/bin/bash # Description: Check for usage of the new tooltip property in PricingDataHeader with corrected file type # Test 1: Search for PricingDataHeader usage with tooltip echo "Searching for PricingDataHeader usage with tooltip:" rg --type ts 'PricingDataHeader.*tooltip' # Test 2: Search for any hardcoded tooltip strings in pricing-related files echo "Searching for hardcoded tooltip strings in pricing-related files:" rg --type ts 'tooltip:' src/core/PricingLength of output: 424
Script:
#!/bin/bash # Description: Extract lines with the tooltip property in src/core/Pricing/data.tsx to verify usage echo "Extracting tooltip property usages in src/core/Pricing/data.tsx:" rg 'tooltip:\s*' src/core/Pricing/data.tsx -A 2Length of output: 402
src/core/Pricing/data.tsx (1)
Line range hint
230-276: Summary: Tooltips successfully added to pricing dataThe changes in this file successfully implement tooltips for the "Channels" and "Connections" entries in the
consumptionDataarray. These additions enhance the user experience by providing concise explanations and links to further documentation directly within the pricing interface.The implementation is consistent across both entries and aligns well with the PR objectives. No unintended changes or regressions were introduced.
To ensure consistency across the codebase, let's verify if similar tooltips have been added to other relevant sections:
This will help us confirm if the tooltip implementation is consistent across all relevant pricing components.
✅ Verification successful
All tooltip implementations in pricing components are consistent
The tooltips have been successfully added to the "Channels" and "Connections" entries in the
consumptionDataarray. A search within thesrc/core/Pricingdirectory confirms that these are the only tooltip implementations, ensuring consistency across the pricing components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tooltip implementations in pricing-related files rg --type typescript --type tsx 'tooltip:.*<p>' src/core/PricingLength of output: 101
Script:
#!/bin/bash # Search for other tooltip implementations in pricing-related TypeScript files by file extension rg 'tooltip:.*<p>' --glob '*.ts' --glob '*.tsx' src/core/PricingLength of output: 66
src/core/Pricing/PricingCards.tsx (1)
2-2: LGTM: Import statements are well-organized.The separation of the
throttleimport and the addition of theTooltipimport improve code organization and support the new tooltip functionality.Also applies to: 9-9
src/core/Tooltip.tsx (4)
36-36: LGTM!The inclusion of the
orientationproperty in thepositionstate enhances tooltip positioning logic effectively.
Line range hint
50-84:
LGTM!The logic for determining the tooltip's orientation based on viewport boundaries is well-implemented.
87-87: LGTM!Resetting the
positionstate when the tooltip is closed maintains the default orientation.
138-138: Ensure consistent fade-out behavior for interactive tooltipsIn the
onMouseLeavehandler, consider verifying that thecursorTowardsTooltipfunction accurately detects when the cursor moves toward the tooltip in all orientations. This will ensure the tooltip remains visible when intended.
jamiehenson
force-pushed
the
pricing-accordion-card-changes
branch
from
8ff30a8 to
de9ec70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/core/Tooltip.tsx (1)
Line range hint
50-84: Improved positioning logic with orientationThe new positioning logic is a significant improvement. It intelligently adjusts the tooltip's position and orientation based on viewport constraints, ensuring better visibility and user experience.
A minor suggestion for improvement:
Consider extracting the positioning logic into a separate function for better readability and maintainability. This would make the useEffect hook cleaner and the positioning logic more reusable if needed elsewhere.
Example:
const calculatePosition = (floatingRect: DOMRect, referenceRect: DOMRect) => { // ... positioning logic ... return { x, y, orientation }; }; // In useEffect const newPosition = calculatePosition(floatingRect, referenceRect); setPosition(newPosition);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- package.json (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (2 hunks)
- src/core/Pricing/types.ts (1 hunks)
- src/core/Tooltip.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/core/Pricing/PricingCards.tsx
- src/core/Pricing/data.tsx
- src/core/Pricing/types.ts
🧰 Additional context used
🔇 Additional comments (5)
src/core/Tooltip.tsx (5)
36-36: LGTM: Addition of orientation to position stateThe inclusion of the
orientationproperty in thepositionstate is a good addition. It provides a clear way to track and manage the tooltip's orientation, which is crucial for the new positioning logic.
87-87: LGTM: Consistent reset of position stateResetting the orientation to "top" when closing the tooltip is a good practice. It ensures consistency with the initial state and prevents any potential issues if the tooltip is reopened in a different context.
105-122: Improved cursor position detection with orientation supportThe renamed
cursorTowardsTooltipfunction is a significant improvement. It now correctly handles cursor position detection for all orientations, addressing the issue mentioned in the previous review.Great job on:
- Using
heightinstead ofwidthfor vertical positioning in 'left' and 'right' orientations.- Implementing separate logic for each orientation, improving accuracy and readability.
This change enhances the tooltip's interactive behavior, especially for edge cases where the tooltip changes orientation.
138-140: LGTM: Enhanced interactive behaviorThe updated
onMouseLeaveevent handler now correctly utilizes thecursorTowardsTooltipfunction. This change improves the tooltip's interactive behavior, preventing premature closing when the cursor moves towards the tooltip, regardless of its orientation.This enhancement aligns well with the interactive tooltip functionality mentioned in the PR objectives.
Line range hint
1-190: Overall assessment: Significant improvements to Tooltip componentThis update to the Tooltip component successfully addresses the PR objectives:
- It adds support for multiple tooltip orientations.
- It enhances the interactive behavior of the tooltip.
Key improvements:
- Introduction of orientation-based positioning logic.
- Improved cursor position detection for all orientations.
- Enhanced interactive behavior preventing premature tooltip closing.
The changes are well-implemented, address previous review comments, and align with the provided AI summary. Great job on improving the functionality and user experience of the Tooltip component!
jamiehenson
force-pushed
the
pricing-accordion-card-changes
branch
from
de9ec70 to
ae871fc
Compare
|
I added a bit extra to try and make interactive hovers a bit more stable - by determining whether the cursor is within the original triggering element upon leaving the tooltip after a short time has passed. Not 100% but it helps |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/core/Tooltip.tsx (2)
Line range hint
50-84: Improved tooltip positioning logicThe new positioning logic is a significant improvement. It handles various edge cases and ensures the tooltip stays within the viewport. The dynamic orientation setting based on available space is particularly useful.
However, consider extracting the positioning logic into a separate function for better readability and maintainability.
Consider refactoring the positioning logic into a separate function:
const calculatePosition = (floatingRect: DOMRect, referenceRect: DOMRect) => { let x = /* ... */; let y = /* ... */; let orientation = "top"; // ... (rest of the positioning logic) return { x, y, orientation }; }; // In useEffect if (floatingRect && referenceRect) { const newPosition = calculatePosition(floatingRect, referenceRect); setPosition(newPosition); }This refactoring would make the useEffect hook cleaner and the positioning logic more reusable.
178-180: LGTM: Enhanced tooltip fade-out behaviorThe addition of the onMouseLeave event to the tooltip itself is a good improvement. The delay before checking if the cursor is within the trigger enhances the user experience, especially for interactive tooltips.
Consider extracting the delay value to a constant for better maintainability:
const FADE_OUT_DELAY = 250; // ... onMouseLeave={(event) => setTimeout(() => fadeOutIfNotWithinTrigger(event), FADE_OUT_DELAY) }This change would make it easier to adjust the delay if needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- package.json (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (2 hunks)
- src/core/Pricing/types.ts (1 hunks)
- src/core/Tooltip.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/core/Pricing/PricingCards.tsx
- src/core/Pricing/data.tsx
- src/core/Pricing/types.ts
🧰 Additional context used
🔇 Additional comments (6)
src/core/Tooltip.tsx (6)
36-36: LGTM: Enhanced position state with orientationThe addition of the
orientationproperty to the position state is a good improvement. It allows for more flexible and precise positioning of the tooltip, which is consistent with the orientation-based logic introduced later in the component.
87-87: LGTM: Proper reset of tooltip stateResetting the orientation to "top" when the tooltip is closed is a good practice. It ensures a consistent initial state for the tooltip, which can prevent unexpected behavior when the tooltip is reopened.
105-129: LGTM: Improved cursor position detectionThe renamed
cursorTowardsTooltipfunction with updated logic is a significant improvement. The switch statement for different orientations makes the code more readable and maintainable. The logic for each orientation is correct and uses the appropriate dimensions.It's great to see that the past review comment about using 'height' instead of 'width' in some calculations has been addressed.
131-144: LGTM: Enhanced fade-out behaviorThe new
fadeOutIfNotWithinTriggerfunction is a valuable addition. It improves the user experience by ensuring the tooltip fades out when the cursor is outside the trigger element. The logic is clear, concise, and easy to understand.
152-154: LGTM: Improved tooltip behavior on mouse leaveThe updated onMouseLeave event handler now uses the
cursorTowardsTooltipfunction to determine whether to initiate fade-out. This change improves the tooltip's behavior, especially for interactive tooltips, providing a more intuitive user experience.
Line range hint
1-198: Overall assessment: Excellent improvements to the Tooltip componentThis PR introduces significant enhancements to the Tooltip component, including:
- Dynamic orientation based on available space
- Improved positioning logic
- Enhanced fade-out behavior
- Better handling of interactive tooltips
These changes align well with the PR objectives and greatly improve the component's functionality and user experience. The code is generally well-structured and readable, with only minor suggestions for improvement (mainly related to extracting some logic into separate functions for better maintainability).
Great job on addressing previous review comments and implementing these valuable updates!
jamiehenson
force-pushed
the
pricing-accordion-card-changes
branch
from
ae871fc to
bf53fd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/core/Tooltip.tsx (3)
Line range hint
50-84: LGTM: Improved tooltip positioning logicThe new logic for determining tooltip orientation and adjusting its position is well-implemented. It effectively prevents tooltips from being cut off at screen edges, which enhances user experience.
A minor optimization suggestion:
Consider extracting the viewport dimensions calculation outside the
ifblock to avoid recalculation:+ const viewportWidth = window.innerWidth; + const viewportHeight = window.innerHeight; if (floatingRect && referenceRect) { - const viewportWidth = window.innerWidth; - const viewportHeight = window.innerHeight; // ... rest of the code }This small change could slightly improve performance, especially if the effect runs frequently.
131-144: LGTM: Enhanced interactive tooltip behaviorThe new
fadeOutIfNotWithinTriggerfunction is a great addition that improves the interactive behavior of the tooltip. It provides a smoother user experience by preventing abrupt tooltip disappearances when the cursor leaves the trigger element.A minor optimization suggestion:
Consider using destructuring to make the code more concise:
- const withinBounds = - clientX >= x && - clientX <= x + width && - clientY >= y && - clientY <= y + height; + const withinBounds = + clientX >= x && clientX <= x + width && + clientY >= y && clientY <= y + height;This change doesn't affect functionality but slightly improves readability.
178-180: LGTM: Enhanced interactive tooltip behaviorThe addition of the
onMouseLeaveevent handler with a delayed call tofadeOutIfNotWithinTriggeris an excellent improvement. It enhances the interactive behavior of the tooltip, allowing users to move the cursor to the tooltip content without it immediately disappearing. This change aligns well with the PR objectives and the author's comments about improving the stability of interactive hovers.A minor suggestion for consistency:
Consider extracting the delay value to a constant at the top of the component:
const FADE_OUT_DELAY = 250; // Then use it in both places: setTimeout(() => fadeOutIfNotWithinTrigger(event), FADE_OUT_DELAY) // and in the initiateFadeOut function: fadeOutTimeoutRef.current = setTimeout(() => { setOpen(false); setFadeOut(false); }, FADE_OUT_DELAY);This change would make it easier to adjust the delay consistently across the component if needed in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/core/Table/__snapshots__/Table.stories.tsx.snapis excluded by!**/*.snapsrc/core/Tooltip/__snapshots__/Tooltip.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
- package.json (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (2 hunks)
- src/core/Pricing/types.ts (1 hunks)
- src/core/Tooltip.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/core/Pricing/PricingCards.tsx
- src/core/Pricing/data.tsx
- src/core/Pricing/types.ts
🧰 Additional context used
🔇 Additional comments (4)
src/core/Tooltip.tsx (4)
36-36: LGTM: Enhanced position state with orientationThe addition of the
orientationproperty to the position state is a good improvement. It allows for more flexible positioning of the tooltip, which is consistent with the overall changes described in the PR objectives.
87-87: LGTM: Proper state reset on tooltip closeResetting the position state, including the orientation, when the tooltip is closed is a good practice. It ensures a clean state for the next tooltip opening, preventing potential issues with stale positioning data.
105-129: LGTM: Improved cursor position detectionThe
cursorTowardsTooltipfunction has been significantly improved:
- It now correctly handles all four orientations (top, left, right, bottom).
- The use of a switch statement enhances readability and maintainability.
- The early return for a null ref is a good defensive programming practice.
These changes effectively address previous review comments and improve the overall functionality of the tooltip.
148-160: LGTM: Improved tooltip trigger styling and behaviorThe changes to the tooltip trigger element are well-implemented:
- The additional class names provide more flexibility in styling, which is beneficial for maintaining consistent design across the application.
- The updated
onMouseLeavehandler, using the newcursorTowardsTooltipfunction, improves the interactive behavior of the tooltip, especially for the case wheninteractiveis true.These improvements align well with the PR objectives and enhance the overall user experience.
Jira Ticket Link / Motivation
https://ably.atlassian.net/jira/software/projects/WEB/boards/172?selectedIssue=WEB-4041
Motivated by this request here: https://ably-real-time.slack.com/archives/C019XP5SYB0/p1728566555191379?thread_ts=1728019882.765889&cid=C019XP5SYB0
Summary of changes
How do you manually test this?
https://ably.github.io/ably-ui/?path=/story/features-pricing-cards--consumption
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
Improvements
Version Update