[WEB-4100] Add LinkButton component, migrate new buttons to PricingCards component #575
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 3
🧹 Outside diff range and nitpick comments (8)
src/core/Button.tsx (3)
68-79: Consider memoizing the commonButtonProps functionThe function generates class names based on props. Consider memoizing it to prevent unnecessary recalculations, especially when used in both Button and LinkButton components.
-export const commonButtonProps = (props: ButtonPropsBase) => { +export const commonButtonProps = React.memo((props: ButtonPropsBase) => { const { variant = "primary", size, leftIcon, rightIcon, className } = props; return { className: cn( buttonClasses[variant][size ?? "md"], { [iconModifierClasses[size ?? "md"].left]: leftIcon }, { [iconModifierClasses[size ?? "md"].right]: rightIcon }, className, ), }; -}; +});
81-92: Enhance accessibility for button contentWhile the interior layout is consistent, consider adding aria-hidden to decorative icons and proper spacing for screen readers.
export const commonButtonInterior = ( props: PropsWithChildren<ButtonPropsBase>, ) => { const { leftIcon, rightIcon, children } = props; return ( <> - {leftIcon ? <Icon name={leftIcon} /> : null} + {leftIcon ? <Icon name={leftIcon} aria-hidden="true" /> : null} {children} - {rightIcon ? <Icon name={rightIcon} /> : null} + {rightIcon ? <Icon name={rightIcon} aria-hidden="true" /> : null} </> ); };
105-108: Consider removing type assertion for better type safetyThe type assertion on rest props could be avoided since ButtonProps already includes ButtonHTMLAttributes.
- {...(rest as React.ButtonHTMLAttributes<HTMLButtonElement>)} + {...rest}src/core/LinkButton/LinkButton.stories.tsx (3)
5-16: Good story configuration with clear documentation!Consider adding more detailed usage examples in the description, such as when to use LinkButton vs Button.
20-33: Well-structured stories with security best practices!Consider adding more examples showcasing different variants and sizes.
export const WithIcon = Template.bind({}); WithIcon.args = { href: "#", variant: "primary", children: "Button with Icon", leftIcon: "arrow-right" };
35-40: Add interaction testing for disabled stateConsider adding an interaction test to verify that clicks are prevented when disabled.
// Add to story metadata parameters: { play: async ({ canvasElement }) => { const canvas = within(canvasElement); const button = canvas.getByText('Disabled Link Button'); await userEvent.click(button); // Verify no navigation occurred }, }src/core/styles/buttons.css (1)
126-132: Consider consolidating disabled stylesYou've introduced new
.ui-button-disabledclasses while maintaining similar styles in.ui-button-disabled-base. This could lead to confusion about which disabled styles to use.Consider consolidating these styles to avoid duplication:
- .ui-button-disabled { - @apply select-none pointer-events-none bg-gui-unavailable text-gui-unavailable-dark hover:bg-gui-unavailable; - } - .ui-theme-dark .ui-button-disabled { - @apply select-none pointer-events-none bg-gui-unavailable-dark text-gui-unavailable hover:bg-gui-unavailable-dark; - } .ui-button-disabled-base { - @apply disabled:cursor-not-allowed disabled:bg-gui-unavailable disabled:text-gui-unavailable-dark disabled:hover:bg-gui-unavailable; + @apply disabled:cursor-not-allowed disabled:bg-gui-unavailable disabled:text-gui-unavailable-dark disabled:hover:bg-gui-unavailable disabled:select-none disabled:pointer-events-none; }src/core/Pricing/PricingCards.tsx (1)
188-190: Consider explicit variant mappingThe
variant="primary"is hardcoded whileclassNameis being passed fromcta. This might lead to style conflicts.Consider allowing variant override through props:
<LinkButton className={cn("w-full", cta.className)} - variant="primary" + variant={cta.variant ?? "primary"} href={cta.url}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/Button.tsx(4 hunks)src/core/LinkButton.tsx(1 hunks)src/core/LinkButton/LinkButton.stories.tsx(1 hunks)src/core/Pricing/PricingCards.tsx(2 hunks)src/core/Pricing/data.tsx(1 hunks)src/core/styles/buttons.css(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/Pricing/data.tsx
🔇 Additional comments (3)
src/core/Button.tsx (1)
Line range hint 10-31: Well-structured type definition with clear documentation!
Good architectural decision to extract ButtonPropsBase as it enables proper type sharing between Button and LinkButton components while maintaining clear documentation.
src/core/styles/buttons.css (2)
31-35: Nice improvement on button hover animation!
The new gradient animation with bg-[size:300%] and hover:bg-right creates a smooth fade effect for priority buttons. This matches the PR objective of improving hover behavior with an animated fade effect.
23-27: Good refactor of disabled state handling
Moving disabled:cursor-not-allowed to the specific disabled base classes improves organization and maintainability. This change centralizes disabled state styling.
jamiehenson
force-pushed
the
link-button-component
branch
from
15ceed5 to
e57710c
Compare
jamiehenson
force-pushed
the
link-button-component
branch
from
21b1c13 to
3907226
Compare
jamiehenson
force-pushed
the
link-button-component
branch
2 times, most recently
from
3c4d5c2 to
3b44850
Compare
jamiehenson
force-pushed
the
link-button-component
branch
from
3b44850 to
4529c14
Compare
Jira Ticket Link / Motivation
Supporting component for the ongoing docs navigation rewrite.
The
LinkButtoncomponent is functionally identical toButton, but is based around anatag instead of abuttontag - this means that thinks like linking are natively supported. I considered having a prop modifyButton, but figured that introduces unnecessary complexity when the two elements serve different roles.Also contains fixes for incorrect hover colouring on
PricingCardsbuttons, and a fix to the hover behaviour for high priority buttons by taking the animated fade from the legacy buttons.Note, the Storybook test runner is timing out here and I'm not sure why. As far as I can see all the stories and components display just fine.
How do you manually test this?
Book
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
LinkButtoncomponent that behaves like a button while rendering as a link.PricingCardsto utilize the newLinkButtonfor call-to-action buttons.Bug Fixes
Style
Documentation
LinkButtoncomponent, showcasing its various states.