14.6.4 - Interactive tooltip, accordion/expander/featuredlink updates #498
Conversation
WalkthroughThis pull request includes updates across multiple files, primarily focusing on version updates, modifications to component styling, and the introduction of new stories for documentation. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 5
🧹 Outside diff range and nitpick comments (10)
src/core/Pricing/PricingCards.stories.tsx (1)
21-21: LGTM! Consider using Tailwind CSS color variables for consistency.The change to darker gradient colors (
from-blue-800 to-pink-800) for the dark theme is appropriate and likely improves contrast. Good job!For better maintainability and consistency across the project, consider using Tailwind CSS color variables if they are defined in your project. For example:
- className={`${theme === "dark" ? "bg-gradient-to-r from-blue-800 to-pink-800" : ""} p-32`} + className={`${theme === "dark" ? "bg-gradient-to-r from-primary-800 to-secondary-800" : ""} p-32`}This assumes you have
primaryandsecondarycolor variables defined in your Tailwind config. If not, the current implementation is fine.src/core/FeaturedLink.tsx (1)
88-88: Consistent group hover styling and refactoring opportunityThe update from
group-hover:left-0togroup-hover/featured-link:left-0in the Icon'sadditionalCSSprop is consistent with the earlier changes. This modification ensures that the hover effect is specifically tied to the FeaturedLink component, maintaining the existing functionality while improving style isolation.Consider refactoring the common styles between the reverse and non-reverse cases to improve code maintainability. You could create a variable for the common classes and only vary the direction-specific classes. For example:
const commonIconClasses = `align-middle relative -top-1 transition-[left] ${disabled ? "" : "group-hover/featured-link:left-0"}`; const reverseIconClasses = `${commonIconClasses} mr-8 -right-4 transform rotate-180`; const forwardIconClasses = `${commonIconClasses} ml-8 -left-4`; // Then use these in the respective Icon components additionalCSS={reverseIconClasses} // and additionalCSS={forwardIconClasses}This refactoring would make it easier to maintain consistent styling between the two cases and reduce code duplication.
src/core/Expander.tsx (1)
85-85: Improved flexibility, but consider further enhancementsThe change improves the component's flexibility by conditionally applying the top margin. However, there are a few points to consider:
The condition
heightThreshold === 0might be too specific. Consider using a more flexible condition, such asheightThreshold <= someSmallValue, to account for cases where a very small threshold should also trigger this behavior.The line is quite long, which may reduce readability. Consider splitting it for better maintainability:
className={` ${heightThreshold === 0 && !expanded ? "" : "mt-16"} cursor-pointer font-bold text-gui-blue-default-light hover:text-gui-blue-hover-light ${controlsClassName ?? ""} `}
- Ensure to test this change with various
heightThresholdvalues and in different layout contexts to verify that removing the margin doesn't negatively impact the overall layout.src/core/Pricing/data.tsx (2)
20-26: Approved changes with a suggestion for consistencyThe updates to the "Free" plan provide more precise information and align with industry terminology. The change from "Capacity" to "Limits" and the addition of specific limitations improve clarity for users.
For consistency with other plans, consider adding "Unlimited messages/month" instead of "6M messages/month". This would highlight the generous nature of the free plan while still setting clear expectations.
126-126: Approved changes with a suggestion for clarityThe updates to the Pro plan, including the consistent use of "Limits" and the improved support SLA, enhance the value proposition for users.
Consider clarifying the available support channels for the 2-hour SLA. The removal of "email" might imply multiple support options. If this is the case, it would be beneficial to specify them (e.g., "2 hour support SLA (email & chat)").
Also applies to: 135-135
src/core/Pricing/PricingCards.tsx (3)
145-149: LGTM: Enhanced FeaturedLink styling with smooth transitionsThe updated
additionalCSSprop improves the visual feedback on hover and maintains consistent theming. The use of responsive classes is a good practice.Consider extracting the long string of classes into a separate constant or utility function to improve readability. For example:
const getFeaturedLinkClasses = (t: (color: ColorClass) => string) => ` absolute sm:-translate-x-96 sm:opacity-0 sm:group-hover:translate-x-0 duration-300 delay-0 sm:group-hover:delay-100 sm:group-hover:opacity-100 transition-[transform,opacity] font-medium ui-text-p3 ${t("text-neutral-500")} hover:${t("text-neutral-000")} `.trim(); // Usage additionalCSS={getFeaturedLinkClasses(t)}
153-157: LGTM: Added ellipsis with smooth transitionThe addition of the ellipsis with transition effects enhances the visual feedback and complements the FeaturedLink hover state. The use of absolute positioning and transforms for the swap animation is a good approach.
Consider using a more semantic HTML element for the ellipsis, such as
<span>instead of<div>, as it doesn't contain any block-level content. For example:<span className={`absolute sm:translate-x-0 sm:opacity-100 sm:group-hover:translate-x-96 sm:group-hover:opacity-0 duration-200 delay-100 sm:group-hover:delay-0 transition-[transform,opacity] leading-6 tracking-widen-0.15 font-light text-p3 ${t("text-neutral-800")}`} > ••• </span>
147-149: Improved robustness in icon color selectionThe use of the nullish coalescing operator for fallback color selection is a good practice, improving the component's robustness.
Consider using a theme-aware fallback color instead of the hardcoded "text-white". This would ensure better consistency across different themes. For example:
iconColor={t( listItemColors?.foreground ?? "text-neutral-000" )}This way, the fallback color will be consistent with the current theme settings.
src/core/Tooltip.tsx (2)
22-22: Consider providing a default value for theinteractiveprop.While
interactiveis an optional prop, specifying a default value can enhance clarity and prevent unintended behavior when the prop is undefined.You can set a default value in the destructuring:
const Tooltip = ({ children, triggerElement, triggerProps, tooltipProps, theme = "dark", - interactive, + interactive = false, ...rest }: PropsWithChildren<TooltipProps>) => {
94-105: Ensure consistent behavior ofcursorHeadsNorthacross devices.The
cursorHeadsNorthfunction relies on mouse events and may not behave as expected on touch devices or in different browsers. Consider adding checks or fallbacks for touch events to ensure consistent tooltip behavior across all user devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
src/core/Accordion/__snapshots__/Accordion.stories.tsx.snapis excluded by!**/*.snapsrc/core/Expander/__snapshots__/Expander.stories.tsx.snapis excluded by!**/*.snapsrc/core/FeaturedLink/__snapshots__/FeaturedLink.stories.tsx.snapis excluded by!**/*.snapsrc/core/Pricing/__snapshots__/PricingCards.stories.tsx.snapis excluded by!**/*.snapsrc/core/ProductTile/__snapshots__/ProductTile.stories.tsx.snapis excluded by!**/*.snapsrc/core/Tooltip/__snapshots__/Tooltip.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
- package.json (1 hunks)
- src/core/Accordion.tsx (2 hunks)
- src/core/Expander.tsx (1 hunks)
- src/core/Expander/Expander.stories.tsx (1 hunks)
- src/core/FeaturedLink.tsx (3 hunks)
- src/core/Pricing/PricingCards.stories.tsx (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (8 hunks)
- src/core/Pricing/types.ts (1 hunks)
- src/core/Tooltip.tsx (6 hunks)
- src/core/Tooltip/Tooltip.stories.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/core/Pricing/types.ts
🔇 Additional comments (10)
src/core/FeaturedLink.tsx (2)
57-59: Improved specificity in group stylingThe change from
grouptogroup/featured-linkin the class name is a good improvement. This modification aligns with Tailwind CSS best practices for component-specific styling, allowing for more precise targeting of styles and reducing the risk of unintended style conflicts with other components.
77-77: Consistent group hover styling for reverse iconThe update from
group-hover:right-0togroup-hover/featured-link:right-0in the Icon'sadditionalCSSprop is consistent with the earlier change to the group class. This modification ensures that the hover effect is specifically tied to the FeaturedLink component, maintaining the existing functionality while improving style isolation.src/core/Tooltip/Tooltip.stories.tsx (1)
44-77: LGTM! Well-structured story demonstrating the interactive Tooltip.The new 'Interactive' story effectively showcases the usage of the Tooltip component with the 'interactive' prop. The layout is consistent with other stories, providing a clear comparison between dark and light themes.
Consider adding a comment about the 'ui-link' class.
The interactive link within the Tooltip uses the 'ui-link' class. It might be helpful to add a brief comment explaining this class or referencing where it's defined for better maintainability.
Verify the interactive functionality.
While the story structure looks good, it's important to ensure that the interactive functionality works as expected across different browsers and devices.
To verify the interactive functionality, you can use the Storybook development environment:
Once Storybook is running, navigate to the Tooltip stories and test the following:
- Hover over the Tooltip to reveal its content.
- Move the cursor into the Tooltip without it disappearing.
- Click on the link within the Tooltip to ensure it's clickable.
- Verify that the behavior is consistent for both dark and light themes.
src/core/Accordion.tsx (3)
123-123: LGTM! Verify visual consistency across themes.The addition of
mb-16class for non-transparent themes is a good improvement to the spacing. This change aligns with the PR objectives of updating the accordion component.To ensure visual consistency, please verify the appearance of the accordion in both transparent and non-transparent themes. Consider adding before/after screenshots to the PR description for easier review.
138-138: LGTM! Verify spacing consistency across themes.The removal of the conditional
pt-16class standardizes the top padding across all themes, which can lead to more consistent spacing.Please ensure that this change, in combination with the added
mb-16on the button, provides the desired spacing for the accordion content across all themes. Consider the following:
- Test the accordion with various content lengths to ensure proper spacing.
- Verify that the removal of
pt-16doesn't create any unintended visual issues, especially for non-transparent themes.- Add before/after screenshots to the PR description showcasing the accordion in different themes and states (open/closed) to facilitate easier review of these spacing changes.
Line range hint
1-194: Overall LGTM! Remember to complete the frontend checklist.The changes to the Accordion component improve the spacing consistency and align with the PR objectives. Both modifications have been approved, but require visual verification.
As a reminder, please ensure you complete the frontend checklist mentioned in the PR description, including:
- Providing before/after screenshots
- Testing across different platforms and browsers
- Ensuring compliance with accessibility standards
These steps will help validate the visual and functional impact of the changes across various scenarios.
src/core/Pricing/data.tsx (1)
75-75: Approved change for consistencyThe update from "Capacity" to "Limits" in the Standard plan maintains consistency with the Free plan and improves clarity.
src/core/Pricing/PricingCards.tsx (2)
80-82: LGTM: Improved styling for price contentThe addition of the
ui-text-p3class to the price content wrapper improves styling consistency and aligns with the PR's objective of updating UI components.
Line range hint
1-174: Overall assessment: Well-implemented UI enhancementsThe changes in this file successfully achieve the PR's objective of enhancing UI components, specifically for pricing cards. The modifications improve visual presentation, interaction, and responsiveness. The code maintains good practices in terms of theming and responsive design.
Key improvements:
- Enhanced styling for price content
- Smooth transition effects for interactive elements
- Addition of ellipsis for improved visual feedback
- More robust color handling
While there are no major issues, consider the minor suggestions provided in the previous comments to further refine the code quality and maintainability.
src/core/Tooltip.tsx (1)
151-151: Conditionally applypointer-eventsbased oninteractiveprop.The application of the
pointer-events-noneclass based on theinteractiveprop is a good approach to control user interaction with the tooltip content.
| export const ZeroHeightContent = { | ||
| render: () => ( | ||
| <Expander fadeClassName="from-transparent" heightThreshold={0}> | ||
| {longContentInner} | ||
| </Expander> | ||
| ), | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| story: "A fully collapsed body of content.", | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the ZeroHeightContent story for better demonstration and testing.
The new story effectively showcases a fully collapsed Expander component. However, there are a few suggestions to improve its demonstration and testing capabilities:
- Add a play function to simulate user interaction and verify the component's behavior when expanding from a fully collapsed state.
- Consider adding a custom control to toggle the
heightThresholdbetween 0 and a larger value to demonstrate the difference in behavior. - The fade effect might not be visible in the fully collapsed state. Consider adding a note in the documentation about this or adjusting the initial state to show a small portion of content.
Here's an example of how you could enhance the story:
export const ZeroHeightContent = {
render: (args) => (
<Expander fadeClassName="from-transparent" heightThreshold={args.heightThreshold}>
{longContentInner}
</Expander>
),
args: {
heightThreshold: 0,
},
argTypes: {
heightThreshold: {
control: { type: 'number' },
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const expander = canvas.getByTestId("expander-container");
// Check initial collapsed state
await expect(expander).toHaveStyle({ height: '0px' });
// Simulate expanding
const control = canvas.getByTestId("expander-controls");
await userEvent.click(control);
// Check expanded state
await waitFor(() => expect(expander).not.toHaveStyle({ height: '0px' }));
},
parameters: {
docs: {
description: {
story: "A fully collapsed body of content. Use the heightThreshold control to toggle between fully collapsed and partially visible states.",
},
},
},
};This enhancement allows for better interaction in Storybook and provides a more comprehensive test of the component's functionality.
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
fd92135 to
c148aa9
Compare
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
c148aa9 to
a4747da
Compare
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
a4747da to
0e18c3b
Compare
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
54c4de9 to
21a78bc
Compare
Jira Ticket Link / Motivation
Summary of changes
How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Release Notes
New Features
@ably/uipackage to14.6.4.interactiveprop for the Tooltip component, enhancing user interaction.Expandercomponent to demonstrate theZeroHeightContentfunctionality.Improvements
Bug Fixes