[WEB-3841] Pricing-oriented improvements #492
Conversation
WalkthroughThe changes include updates to the Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (1)
Additional comments not posted (1)
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 (
|
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
3ce026b
to
7b98b73
Compare
|
@coderabbitai review |
Actions performedReview triggered.
|
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
b8335a9
to
fc2e10f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (11)
src/core/Accordion/types.ts (1)
38-38: LGTM! Consider updating documentation.The addition of the optional
borderproperty toAccordionThemeColorsis a good enhancement for customization.Consider updating the component's documentation to reflect this new theming option. This will help developers understand and utilize the new
borderproperty effectively.src/core/Tooltip/Tooltip.stories.tsx (2)
16-24: Improved layout and theme presentation.The changes to the Central story significantly enhance the presentation of the Tooltip component:
- The new grid layout with two columns provides a clear side-by-side comparison of dark and light themes.
- The added styling for each theme (background color for dark theme) improves visual context.
- The consistent sizing and centering of tooltips in both themes allow for easy comparison.
These modifications align well with the PR objectives of improving design requirements for the upcoming pricing page.
For consistency, consider adding a label above each tooltip to indicate the theme, similar to how you've labeled the columns. This could be achieved by adding a
<p>element above each tooltip with the theme name.<div className="grid grid-cols-2 gap-16 w-[600px]"> <div className="font-mono text-center">dark</div> <div className="font-mono text-center">light</div> <div className="w-256 h-256 bg-neutral-1300 flex items-center justify-center m-24 border mx-auto rounded-lg"> + <p className="absolute top-2 left-2 text-white">Dark theme</p> <Tooltip>{args.children}</Tooltip> </div> <div className="w-256 h-256 flex items-center justify-center m-24 border mx-auto rounded-lg"> + <p className="absolute top-2 left-2">Light theme</p> <Tooltip theme="light">{args.children}</Tooltip> </div> </div>This addition would further improve the clarity of the story for users viewing the component documentation.
31-39: Consistent improvements and demonstration of left-bound alignment.The changes to the LeftBound story maintain consistency with the Central story while showcasing a different alignment:
- The grid layout and theme presentation are consistent with the Central story, which is good for maintaining a cohesive documentation style.
- The use of
items-centerwithoutjustify-centereffectively demonstrates how the Tooltip behaves when left-aligned.These changes contribute to a more comprehensive showcase of the Tooltip component's versatility.
To make the difference between the Central and LeftBound stories more apparent, consider adding a visual indicator or label that highlights the left-bound nature of this variation. This could be achieved by adding a vertical line or a label to the left of each tooltip. For example:
<div className="grid grid-cols-2 gap-16 w-[600px]"> <div className="font-mono text-center">dark</div> <div className="font-mono text-center">light</div> - <div className="w-256 h-256 bg-neutral-1300 flex items-center m-24 border mx-auto rounded-lg"> + <div className="w-256 h-256 bg-neutral-1300 flex items-center m-24 border mx-auto rounded-lg relative"> + <div className="absolute left-0 top-0 bottom-0 w-1 bg-red-500"></div> + <p className="absolute top-2 left-4 text-white text-sm">Left-bound</p> <Tooltip>{args.children}</Tooltip> </div> - <div className="w-256 h-256 flex items-center m-24 border mx-auto rounded-lg"> + <div className="w-256 h-256 flex items-center m-24 border mx-auto rounded-lg relative"> + <div className="absolute left-0 top-0 bottom-0 w-1 bg-red-500"></div> + <p className="absolute top-2 left-4 text-sm">Left-bound</p> <Tooltip theme="light">{args.children}</Tooltip> </div> </div>This addition would make it immediately clear to users viewing the documentation that this story demonstrates a left-bound variation of the Tooltip.
src/core/Tooltip.tsx (3)
5-6: LGTM! Consider adding JSDoc comments for new props.The new imports and type definitions align well with the component's enhanced functionality. The addition of
triggerElementprop and the default value forthemeimprove the component's flexibility and usability.Consider adding JSDoc comments for the new
triggerElementprop and the updatedthemeprop to improve documentation:/** * Custom element to trigger the tooltip. If not provided, a default info icon will be used. */ triggerElement?: ReactNode; /** * Theme for the tooltip. Defaults to "dark". */ theme?: Theme;Also applies to: 10-10, 12-13, 16-16, 19-19, 24-24, 27-27, 31-31
39-75: LGTM! Consider refactoring for improved readability.The use of
useLayoutEffectfor synchronous layout calculations is appropriate. The positioning logic comprehensively handles edge cases for all sides of the viewport, ensuring the tooltip remains visible.To improve readability and maintainability, consider refactoring the positioning logic into separate functions:
const calculateInitialPosition = (referenceRect: DOMRect, floatingRect: DOMRect) => { return { x: referenceRect.left + referenceRect.width / 2 - floatingRect.width / 2 + window.scrollX, y: referenceRect.top - floatingRect.height - offset + window.scrollY }; }; const adjustForViewportBounds = (position: {x: number, y: number}, floatingRect: DOMRect, referenceRect: DOMRect) => { let { x, y } = position; const viewportWidth = window.innerWidth; const viewportHeight = window.innerHeight; // Adjust for right edge if (x + floatingRect.width > viewportWidth + window.scrollX) { x = viewportWidth + window.scrollX - floatingRect.width - offset; } // Adjust for left edge if (x < window.scrollX) { x = window.scrollX + offset; } // Adjust for top edge if (y < window.scrollY) { y = referenceRect.bottom + offset + window.scrollY; } // Adjust for bottom edge if (y + floatingRect.height > viewportHeight + window.scrollY) { y = referenceRect.top - floatingRect.height - offset + window.scrollY; } return { x, y }; }; useLayoutEffect(() => { if (open) { const floatingRect = floating.current?.getBoundingClientRect(); const referenceRect = reference.current?.getBoundingClientRect(); if (floatingRect && referenceRect) { const initialPosition = calculateInitialPosition(referenceRect, floatingRect); const adjustedPosition = adjustForViewportBounds(initialPosition, floatingRect, referenceRect); setPosition(adjustedPosition); } } else { setPosition({ x: 0, y: 0 }); } }, [open]);This refactoring separates the concerns of initial positioning and viewport boundary adjustments, making the code more modular and easier to understand.
115-135: LGTM! Consider extracting className logic.The use of
createPortalfor rendering the tooltip is appropriate and helps avoid z-index issues. The positioning and styling are consistent with the earlier calculations.To improve readability, consider extracting the complex className string into a separate function:
const getTooltipClassName = (theme: Theme, fadeOut: boolean, additionalClassName?: string) => { return `${t("bg-neutral-1000")} ${t("text-neutral-200")} ui-text-p3 font-medium p-16 rounded-lg pointer-events-none absolute ${ additionalClassName ?? "" } ${fadeOut ? "animate-[tooltipExit_0.25s_ease-in-out]" : "animate-[tooltipEntry_0.25s_ease-in-out]"}`; }; // Usage className={getTooltipClassName(theme, fadeOut, tooltipProps?.className)}This extraction would make the JSX more readable and the className logic more maintainable.
src/core/Accordion/Accordion.stories.tsx (3)
6-9: Improved code structure with reusable text content.The introduction of
loremTextas a separate constant improves code reusability and maintainability. This change is beneficial for potential future localization or content updates.Consider using a template literal for better readability:
-const loremText = - "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin scelerisque congue risus id lobortis. Vivamus blandit dolor at ultricies cursus. Phasellus pharetra nunc erat, quis porttitor mauris faucibus in. Donec feugiat dapibus orci et blandit. Duis eleifend accumsan est nec euismod. Proin imperdiet malesuada lacus, a aliquam eros aliquet nec. Sed eu dolor finibus, sodales nisl a, egestas mi. In semper interdum lacinia. Duis malesuada diam quis purus blandit, sit amet imperdiet neque accumsan. Morbi viverra vitae risus ut pellentesque. Praesent ac blandit augue. Aliquam purus lectus, lacinia in semper vitae, dictum eu felis. Donec vel pulvinar eros, id facilisis neque. Aenean odio arcu, accumsan vel est in, lobortis rhoncus ligula. Pellentesque sit amet odio velit."; +const loremText = ` + Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin scelerisque congue risus id lobortis. + Vivamus blandit dolor at ultricies cursus. Phasellus pharetra nunc erat, quis porttitor mauris faucibus in. + Donec feugiat dapibus orci et blandit. Duis eleifend accumsan est nec euismod. Proin imperdiet malesuada lacus, + a aliquam eros aliquet nec. Sed eu dolor finibus, sodales nisl a, egestas mi. In semper interdum lacinia. + Duis malesuada diam quis purus blandit, sit amet imperdiet neque accumsan. Morbi viverra vitae risus ut pellentesque. + Praesent ac blandit augue. Aliquam purus lectus, lacinia in semper vitae, dictum eu felis. + Donec vel pulvinar eros, id facilisis neque. Aenean odio arcu, accumsan vel est in, lobortis rhoncus ligula. + Pellentesque sit amet odio velit. +`.trim().replace(/\n\s+/g, ' ');This change would make the text content more readable in the source code while preserving the single-line output.
11-15: New textarea element for demonstrating dynamic content.The addition of the
textareaconstant provides a good way to showcase the Accordion's ability to handle resizable content. This is a valuable addition to the story.Consider adding an
aria-labelto the textarea for improved accessibility:const textarea = ( <textarea className="w-full h-256 bg-neutral-700 p-16 rounded-xl leading-relaxed" defaultValue={loremText} + aria-label="Resizable content example" /> );This change would make the textarea more accessible to screen readers.
160-173: New story showcasing resizable content functionality.The addition of the
WithResizableInnerContentstory is an excellent way to demonstrate the Accordion's ability to handle dynamic, resizable content. The description provides clear instructions for manual testing.Consider enhancing the description to provide more context:
export const WithResizableInnerContent = { render: () => AccordionPresentation({ data: dataWithTextarea, }), parameters: { docs: { description: { story: - "Try resizing content within the Accordion entries - the container should respond to the new height accordingly", + "This story demonstrates the Accordion's ability to handle dynamic content. Try resizing the textarea within the Accordion entries - the container should respond to the new height accordingly, showcasing the component's flexibility with varying content sizes.", }, }, }, };This expanded description provides more context about the purpose of the story and what it's demonstrating.
src/core/Expander/Expander.stories.tsx (1)
Line range hint
153-180: Update play function to account for new JSX structureThe play function currently checks for the text content "Away with you, knave." directly. With the new JSX structure, this test might fail as it doesn't account for the added icon.
Consider updating the play function to either:
- Use a more flexible matching method, like:
await expect(canvas.getByTestId("expander-controls")).toHaveTextContent(/Away with you, knave/);
- Or, if you want to be more specific, check for both the text and the icon:
const controlsElement = canvas.getByTestId("expander-controls"); await expect(controlsElement).toHaveTextContent("Away with you, knave."); await expect(controlsElement.querySelector('svg[name="icon-gui-warning"]')).toBeInTheDocument();tailwind.config.js (1)
322-331: LGTM: Smooth tooltip animations addedThe addition of
tooltipEntryandtooltipExitkeyframes is a great improvement for enhancing the user experience with smooth tooltip animations. The entry animation combines a slight upward movement with a fade-in effect, while the exit animation provides a clean fade-out.Consider adding a subtle scale effect to the
tooltipEntryanimation for an even more polished look. For example:tooltipEntry: { - "0%": { transform: "translateY(8px)", opacity: 0 }, - "100%": { transform: "translateY(0)", opacity: 1 }, + "0%": { transform: "translateY(8px) scale(0.95)", opacity: 0 }, + "100%": { transform: "translateY(0) scale(1)", opacity: 1 }, },This would add a slight "pop" effect as the tooltip appears, potentially making it more noticeable to users.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
src/core/Accordion/__snapshots__/Accordion.stories.tsx.snapis excluded by!**/*.snapsrc/core/Expander/__snapshots__/Expander.stories.tsx.snapis excluded by!**/*.snapsrc/core/Meganav/__snapshots__/Meganav.stories.tsx.snapis excluded by!**/*.snapsrc/core/Pricing/__snapshots__/PricingCards.stories.tsx.snapis excluded by!**/*.snapsrc/core/Table/__snapshots__/Table.stories.tsx.snapis excluded by!**/*.snapsrc/core/Tooltip/__snapshots__/Tooltip.stories.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (13)
- package.json (3 hunks)
- src/core/Accordion.tsx (3 hunks)
- src/core/Accordion/Accordion.stories.tsx (4 hunks)
- src/core/Accordion/types.ts (2 hunks)
- src/core/Expander.tsx (1 hunks)
- src/core/Expander/Expander.stories.tsx (2 hunks)
- src/core/Pricing/PricingCards.stories.tsx (1 hunks)
- src/core/Pricing/PricingCards.tsx (1 hunks)
- src/core/Tooltip.tsx (3 hunks)
- src/core/Tooltip/Tooltip.stories.tsx (1 hunks)
- src/core/styles/colors/types.ts (1 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
- tailwind.config.js (2 hunks)
Additional context used
Learnings (1)
src/core/styles/colors/types.ts (1)
Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-09-18T14:08:15.229Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.
Additional comments not posted (34)
src/core/Accordion/types.ts (2)
Line range hint
1-48: Overall, the changes look good and enhance the Accordion component.The additions of the "darkTransparent" theme and the optional
borderproperty improve the component's flexibility and customization options. These changes are well-implemented and maintain backward compatibility.To ensure the best developer experience:
- Verify the implementation of the "darkTransparent" theme in the component's logic and styling.
- Update the component's documentation to reflect the new theming options.
22-27: LGTM! Verify the implementation of the new theme.The addition of the "darkTransparent" theme to the
accordionThemesarray is a good enhancement to the Accordion component's flexibility.Please ensure that the new "darkTransparent" theme is properly implemented in the component's logic and styling. Run the following script to verify the theme usage:
src/core/Pricing/PricingCards.stories.tsx (1)
20-22: Clarify the purpose of the gradient background changeThe background for the dark theme has been updated from a solid color (
bg-neutral-1300) to a gradient (bg-gradient-to-r from-blue-500 to-pink-500). While this change enhances the visual appeal of the Storybook demo, I have a few points to consider:
- Does this gradient background align with the design requirements mentioned in the PR objectives for the upcoming pricing page?
- Have you verified that the gradient background doesn't negatively impact the readability of the PricingCards component in dark mode?
- Is this change intended for the actual application, or is it only for demonstration purposes in Storybook?
To ensure consistency across the codebase, let's check if similar gradient backgrounds are used elsewhere:
Additionally, could you provide before/after screenshots of the PricingCards component with this new background to help assess its visual impact?
Verification successful
Gradient background change is specific to PricingCards.stories.tsx
The search indicates that the gradient background (
bg-gradient-to-r from-blue-500 to-pink-500) is only applied withinsrc/core/Pricing/PricingCards.stories.tsx. This suggests that the change is confined to the Storybook environment and does not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar gradient background classes in other files rg --type typescript --type javascript 'bg-gradient-to-r from-blue-500 to-pink-500'Length of output: 120
Script:
#!/bin/bash # Search for similar gradient background classes in TypeScript and JavaScript files rg --type ts --type tsx --type js --type jsx 'bg-gradient-to-r from-blue-500 to-pink-500'Length of output: 119
Script:
#!/bin/bash # Search for similar gradient background classes in TypeScript and JavaScript files rg --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 'bg-gradient-to-r from-blue-500 to-pink-500'Length of output: 244
src/core/Tooltip/Tooltip.stories.tsx (2)
9-10: Improved default content for better visualization.The update to the
childrenprop with a longer Lorem ipsum text is a good change. It provides a more realistic example of tooltip content, allowing for better visualization of how the component handles longer text. This modification will be helpful for testing and demonstrating the Tooltip's capabilities with various content lengths.
Line range hint
1-41: Summary: Improved Tooltip stories with enhanced layout and theme presentationThe changes made to this file significantly improve the presentation and testing capabilities of the Tooltip component:
- The default args now use a more realistic, longer text content.
- Both Central and LeftBound stories now showcase dark and light themes side by side.
- The new grid layout provides a clear and consistent way to compare different Tooltip variations.
These improvements align well with the PR objectives of enhancing design requirements for the upcoming pricing page. The changes will make it easier for developers and designers to visualize and test the Tooltip component in various scenarios.
Minor suggestions have been made to further improve clarity and consistency across the stories. Overall, these changes represent a positive step in improving the component's documentation and usability.
src/core/styles/colors/utils.ts (1)
29-32: Improved flexibility in color string processingThe changes to the
determineThemeColorfunction enhance its ability to handle more complex color string formats. This is a positive improvement that allows for greater flexibility in the theming system.However, consider the following points:
Backward Compatibility: Ensure that this change doesn't break existing color string formats. It might be helpful to add a comment explaining the expected format of the color string.
Edge Cases: Consider adding checks for edge cases, such as when
splitColor.lengthis exactly 2 or less than 2.Input Validation: It may be beneficial to add input validation to ensure the color string is in the expected format before processing.
To ensure this change doesn't introduce any regressions, let's verify the usage of this function:
Verification successful
Verification Successful: No Issues Found
The updates to the
determineThemeColorfunction have been verified across the codebase. All existing usages conform to the new color string format, and no regressions were detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of determineThemeColor function and color string formats # Search for calls to determineThemeColor echo "Calls to determineThemeColor:" rg --type typescript "determineThemeColor\(" -A 3 # Search for color string definitions that might be affected echo "\nColor string definitions:" rg --type typescript "(const|let|var)\s+\w+\s*=\s*['\"]\w+(-\w+){2,}['\"]" -A 1Length of output: 341
Script:
#!/bin/bash # Description: Check usage of determineThemeColor function and color string formats # Search for calls to determineThemeColor in .ts and .tsx files echo "Calls to determineThemeColor:" rg "determineThemeColor\(" --glob "*.ts" --glob "*.tsx" -A 3 # Search for color string definitions that might be affected in .ts and .tsx files echo "\nColor string definitions:" rg "(const|let|var)\s+\w+\s*=\s*['\"]\w+(-\w+){2,}['\"]" --glob "*.ts" --glob "*.tsx" -A 1Length of output: 1323
src/core/styles/colors/types.ts (2)
15-15: LGTM: Addition of "border" to ColorClassPrefixesThe addition of "border" to the
ColorClassPrefixestype is a good improvement. This change allows for the generation of border color classes, which is a common and useful feature in CSS styling. The modification is consistent with the existing color-related prefixes and aligns with the learning from the previous PR to include only color-related variants and prefixes.This update will enable more flexible color styling options, particularly for border colors, throughout the application.
15-15: Verify usage of updated ColorClass typeWhile the change to
ColorClassPrefixesis straightforward and beneficial, it's important to ensure that this update doesn't cause any unintended effects in other parts of the codebase that use theColorClasstype.Let's check for any potential impacts:
This script will help identify any areas that might need attention due to the
ColorClasstype change.src/core/Expander.tsx (3)
1-7: LGTM: Import statement updated correctly.The addition of
ReactNodeto the import statement is consistent with its usage in the updated type definitions forcontrolsOpenedLabelandcontrolsClosedLabel.
1-16: Summary: Beneficial changes to Expander component.The modifications to the
Expandercomponent, specifically the type changes forcontrolsOpenedLabelandcontrolsClosedLabel, enhance its flexibility by allowing both string and ReactNode inputs. This update aligns with modern React practices and potentially improves the component's reusability.Key points:
- The import statement has been correctly updated to include
ReactNode.- The
ExpanderPropstype definition now allows more diverse inputs for control labels.These changes are positive improvements to the component. However, it's crucial to verify that existing usages of the
Expandercomponent across the codebase are compatible with these new type definitions. The provided verification script will help identify any areas that may need updates.
15-16: Approve type changes and verify usage.The update to allow both
stringandReactNodeforcontrolsOpenedLabelandcontrolsClosedLabelprovides more flexibility in theExpandercomponent. This change is beneficial and aligns with modern React practices.To ensure this change doesn't introduce issues elsewhere, please run the following script to check the usage of the
Expandercomponent across the codebase:Review the results to ensure that all usages of these props are compatible with the new type definition. If any incompatibilities are found, they should be updated accordingly.
Verification successful
Adjust the search script to correctly target TypeScript files and rerun verification.
The previous script encountered issues with unrecognized file types. Please use the updated script below to search for
Expandercomponent usages and the specific props within.tsand.tsxfiles:Please execute this script and provide the output to continue the verification process.
Type changes verified: No issues found with
controlsOpenedLabelandcontrolsClosedLabelusages.The updates allowing
string | ReactNodeforcontrolsOpenedLabelandcontrolsClosedLabelare compatible with all existing usages in the codebase. No further actions are required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the Expander component and check if controlsOpenedLabel or controlsClosedLabel are used. # Search for Expander component usage echo "Searching for Expander component usage:" rg --type typescript --type typescriptreact '<Expander' -A 10 # Search for specific prop usage echo "\nSearching for controlsOpenedLabel usage:" rg --type typescript --type typescriptreact 'controlsOpenedLabel' echo "\nSearching for controlsClosedLabel usage:" rg --type typescript --type typescriptreact 'controlsClosedLabel'Length of output: 579
Script:
#!/bin/bash # Description: Find all usages of the Expander component and check if controlsOpenedLabel or controlsClosedLabel are used. # Search for Expander component usage echo "Searching for Expander component usage:" rg '<Expander' -A 10 -g "*.ts" -g "*.tsx" # Search for specific prop usage echo "\nSearching for controlsOpenedLabel usage:" rg 'controlsOpenedLabel' -g "*.ts" -g "*.tsx" echo "\nSearching for controlsClosedLabel usage:" rg 'controlsClosedLabel' -g "*.ts" -g "*.tsx"Length of output: 4652
package.json (4)
3-3: Version update looks good.The version has been incremented to 14.6.2-dev.7b98b73, which is appropriate for a development version. This aligns with the PR objectives mentioning updates to several components.
34-34: Good addition of @types/react-dom.Adding type definitions for react-dom is a good practice. The version ^18.3.0 is compatible with the react-dom version being used (^18.3.1). This addition will improve type checking and developer experience.
Line range hint
1-95: Overall, the changes look good with one point to verify.The package.json updates include:
- A version bump to a development version.
- Addition of @types/react-dom for improved TypeScript support.
- An update to react-dom.
These changes are generally positive, improving type definitions and keeping dependencies up-to-date. The main point to verify is the necessity and impact of the react-dom update from 18.2 to 18.3.
Please ensure all changes align with the PR objectives and have been thoroughly tested.
82-82: Verify the necessity of updating react-dom.The update of react-dom from ^18.2.0 to ^18.3.1 is generally a good practice. However, please verify:
- Is this update necessary for the current PR objectives?
- Have you tested the application thoroughly to ensure this update doesn't introduce any breaking changes?
- Are there any new features in react-dom 18.3 that this project will utilize?
To help verify the impact of this change, you can run the following script:
src/core/Tooltip.tsx (4)
91-97: LGTM! Smooth fade-out animation implementation.The implementation of the fade-out animation is well done. The use of
setTimeoutwith a 250ms delay ensures that the animation completes before closing the tooltip, providing a smooth user experience.
106-112: LGTM! Flexible trigger element implementation.The conditional rendering of either a custom
triggerElementor the default Icon component provides excellent flexibility for the Tooltip usage. The color determination using thetfunction is consistent with the earlier implementation.
Line range hint
1-141: Overall, excellent improvements to the Tooltip component.The changes in this file significantly enhance the functionality and user experience of the Tooltip component. The implementation of custom trigger elements, improved positioning logic, and smooth animations align well with the PR objectives. The use of
createPortalanduseLayoutEffectdemonstrates a good understanding of React best practices.While there are a few minor suggestions for improvements (such as adding JSDoc comments, refactoring complex logic, and extracting className calculations), the overall implementation is solid and well-thought-out.
Great job on these improvements!
37-38: Verify the correctness of the theme argument indetermineThemeColor.The
tfunction is using "light" as the first argument todetermineThemeColor, but the default theme for the component is set to "dark". This might lead to unexpected color determinations.Please confirm if this is intentional or if it should be changed to use the
themeprop:const t = (color: ColorClass) => determineThemeColor(theme, theme, color);To verify the usage of
determineThemeColor, let's check its implementation:src/core/Accordion/Accordion.stories.tsx (2)
28-31: New dataset with resizable content.The introduction of
dataWithTextareais a good approach to create a dataset for demonstrating the Accordion with resizable content. It maintains the structure of the original data while replacing the content with the textarea element.This change aligns well with the new story for resizable content that has been added.
47-52: Layout adjustment and theme-specific styling.The changes to the
AccordionPresentationcomponent improve the layout and visual consistency:
- The grid layout has been changed from three columns to two, which may improve responsiveness or accommodate larger content.
- A conditional class for dark themes has been added, improving visual contrast for dark-themed accordions.
These changes are positive improvements to the component's presentation.
Please verify that the layout change from three columns to two doesn't negatively impact the visual presentation of the accordions. You can use the following script to check for any other grid column changes in the codebase:
This will help ensure that the column change is consistent with other parts of the UI.
Verification successful
Grid column changes are consistent across the codebase.
The modification to use
grid-cols-2in theAccordionPresentationcomponent aligns with existing grid configurations in other components. No inconsistencies were found that would negatively impact the visual presentation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other grid column changes in the codebase # Search for grid column class changes echo "Searching for grid column class changes:" rg --type tsx --type jsx 'grid-cols-[23]' -C 2Length of output: 167
Script:
#!/bin/bash # Description: Check for other grid column class changes in the codebase # Searching for grid-cols-2 or grid-cols-3 in tsx and jsx files echo "Searching for grid column class changes in tsx and jsx files:" rg 'grid-cols-(2|3)' --glob '*.tsx' --glob '*.jsx' -C 2Length of output: 5740
src/core/Expander/Expander.stories.tsx (3)
4-4: LGTM: Icon import added correctlyThe import for the Icon component has been added appropriately. This import is necessary for the changes made in the
OverriddenControlsstory.
140-145: LGTM: Enhanced visual representation of opened labelThe change to the
controlsOpenedLabelprop improves the visual representation by adding an icon alongside the text. The use of a flex container with gap ensures proper alignment. This update aligns well with the PR objectives of updating components to meet design requirements.
Line range hint
1-180: Summary: Good improvements with a minor test update neededThe changes to this file enhance the visual representation of the Expander component by adding an icon to the opened label. This aligns well with the PR objectives of updating components to meet design requirements. The Icon import and usage are implemented correctly.
The only suggestion is to update the play function to account for the new JSX structure of the
controlsOpenedLabel. This will ensure that the tests continue to pass and accurately reflect the component's behavior.Overall, these changes contribute positively to the "[WEB-3841] Pricing-oriented improvements" pull request.
tailwind.config.js (2)
14-14: LGTM: Enhanced styling flexibility with group-hoverThe addition of the "group-hover" variant to this pattern is a good improvement. It allows for more dynamic styling options, particularly useful for interactive components where you want to change styles based on hovering over a parent element.
Line range hint
1-365: Summary: Tailwind config changes align with PR objectivesThe changes made to
tailwind.config.jsare well-aligned with the PR objectives of improving components for the pricing page. The addition of the "group-hover" variant enhances styling flexibility, which can be particularly useful for interactive components like Accordion and Expander. The new tooltip animations will directly benefit the Tooltip component, providing a smoother and more polished user experience.These improvements contribute to meeting the design requirements for the upcoming pricing page. The changes are focused and purposeful, adhering to the PR's goals without introducing unnecessary modifications.
src/core/Accordion.tsx (8)
51-56: Addition of border properties to the 'transparent' theme looks goodThe inclusion of
borderin thetransparenttheme ensures consistent styling for accordion items when using this theme.
57-62: New 'darkTransparent' theme implemented correctlyThe
darkTransparenttheme is correctly added, providing an option for dark-themed transparent accordions with appropriate styling attributes.
67-67: UpdatedisNonTransparentThemefunction accommodates 'darkTransparent'Modifying the
isNonTransparentThemefunction to include'darkTransparent'ensures that padding and other style considerations are correctly applied based on the theme transparency.
84-88: Enhanced height adjustment usingResizeObserverReplacing the throttled resize handler with a
ResizeObserverimproves performance and responsiveness by directly observing size changes of the content.
90-98: Properly observing and unobservingrowRefThe setup and cleanup of the
ResizeObserverare correctly handled, preventing potential memory leaks and ensuring that the observer is active only when needed.
103-111: Destructuring theme properties including 'border'Including
borderin the destructured properties fromthemeClasses[theme]ensures that all relevant styling attributes are applied to the accordion component based on the selected theme.
119-119: Applying border classes to the accordion row containerAdding the
borderclass to the outerdivcorrectly implements the border styling for each accordion row as defined by the theme.
138-138: Conditional padding applied based on theme transparencyUsing the
isNonTransparentThemecheck to conditionally applypt-16ensures consistent spacing for non-transparent themes while maintaining design expectations for transparent themes.
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
0c36d14
to
bec6a58
Compare
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
b408447
to
c32bf61
Compare
|
|
||
| handleHeight(); | ||
| if (rowRef.current) { | ||
| resizeObserver.observe(rowRef.current); |
There was a problem hiding this comment.
This is a superior api for judging resize events as it also takes into account the resizing of the target element, not just the window. We need this for accordions-inside-of-accordions, as pricing demands
| import throttle from "lodash.throttle"; | ||
|
|
||
| type ExpanderProps = { | ||
| heightThreshold?: number; | ||
| className?: string; | ||
| fadeClassName?: string; | ||
| controlsClassName?: string; | ||
| controlsOpenedLabel?: string; | ||
| controlsClosedLabel?: string; | ||
| controlsOpenedLabel?: string | ReactNode; |
There was a problem hiding this comment.
This allows things like icons to be passed in as well
| @@ -45,110 +45,109 @@ const PricingCards = ({ | |||
| <Fragment key={title.content}> | |||
| {delimiterColumn(index)} | |||
| <div | |||
| className={`flex flex-1 p-1 bg-gradient-to-b ${t("from-neutral-1100")} ${t("to-neutral-1100")} rounded-2xl min-w-[272px]`} | |||
| className={`relative border ${t("border-neutral-1100")} flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group ${delimiter ? "@[520px]:flex-row @[920px]:flex-col" : ""} min-w-[272px] overflow-hidden backdrop-blur`} | |||
There was a problem hiding this comment.
Lot of DOM juggling here but the tldr is a rejigging of the backgrounds so that we can support translucency. Alas we lose the gradient border but I made a call to ditch that in favour of the blur as it's more noticable
| referenceRect.width / 2, | ||
| y: Math.min(floatingRect.height, floatingRect.top) + offset, | ||
| }); | ||
| let x = |
There was a problem hiding this comment.
Sorted all this tooltip instantiation logic out, it was the bare minimum before. Now it takes into account the whole viewport and its own placement with respect to the edges, and is appended to the end of the body.
There was a problem hiding this comment.
The tooltip also now has entry and exit animations
| @@ -26,8 +26,10 @@ export const determineThemeColor = ( | |||
| } else { | |||
| const splitColor = color.split("-"); | |||
|
|
|||
| if (splitColor.length === 3) { | |||
| const [property, palette, variant] = splitColor; | |||
| if (splitColor.length >= 3) { | |||
There was a problem hiding this comment.
This change is to support prefixes like group-hover, whose hyphen messed up the old logic
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
f0e7186
to
39782b8
Compare
|
@kennethkalmer I re-added 🐰's suggestion, it was lost when I force pushed. Btw, you needn't pull down and run Storybook these days, we have review builds now. There's a link in the PR description. |
@jamiehenson OMG this is awesome, I did not even notice |
There was a problem hiding this comment.
Snapshot issues aside, this looks great @jamiehenson. I only tested in Safari, for completeness sake :P
|
Thanks - the Status widget keeps playing havoc with the snapshots, I'll exclude all the Meganav components from snapshotting |
Jira Ticket Link / Motivation
https://ably.atlassian.net/browse/WEB-3841
Summary of changes
The commits are fairly granular - this PR is a series of fixes and bumps for
Accordion,Expander,PricingCardsandTooltipto serve design requirements in the upcoming pricing page.How do you manually test this?
Kick back with a good book
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Release Notes
New Features
darkTransparenttheme for the Accordion component.WithResizableInnerContentadded for the Accordion component.Improvements
Bug Fixes
Documentation