-
Notifications
You must be signed in to change notification settings - Fork 217
Consolidate implementation for payment method pills #4605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR consolidates duplicate styled elements and implementation logic for payment method pill components into a reusable PaymentMethodUnavailablePill
component with an accompanying PaymentMethodPopoverLink
helper component. This reduces code duplication and ensures consistent UX across different pill types while slightly reducing bundle size.
- Creates a new
PaymentMethodUnavailablePill
component that accepts atitle
prop and displays children as popover content - Introduces
PaymentMethodPopoverLink
component with default behavior for links within popovers - Refactors five existing pill components to use the new consolidated implementation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
client/components/payment-method-unavailable-pill/index.js | New consolidated pill component with shared styling and popover link helper |
client/components/payment-method-unavailable-due-conflict-pill/index.js | Refactored to use new consolidated pill component, removing duplicate styles |
client/components/payment-method-missing-currency-pill/index.js | Refactored to use new consolidated pill component, removing duplicate styles |
client/components/payment-method-deprecation-pill/index.js | Refactored to use new consolidated pill component, removing duplicate styles |
client/components/payment-method-capability-status-pill/index.js | Refactored to use new consolidated pill component, removing duplicate styles |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
</StyledPill> | ||
); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PaymentMethodPopoverLink component lacks JSDoc documentation. Consider adding documentation that explains its purpose, props, and default behavior for onClick, target, and rel.
/** | |
* A styled anchor link for use within payment method popovers. | |
* | |
* Renders a link with default behavior to open in a new tab and prevent popover closure on click. | |
* | |
* @param {Object} props - Component props. | |
* @param {React.ReactNode} props.children - The link content. | |
* @param {string} [props.target='_blank'] - The target attribute for the link. Defaults to '_blank'. | |
* @param {string} [props.rel='noreferrer'] - The rel attribute for the link. Defaults to 'noreferrer'. | |
* @param {function} [props.onClick] - Optional click handler. The event's propagation is stopped before calling this handler. | |
* @returns {JSX.Element} The styled anchor link component. | |
*/ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, but happy to get feedback on this one. We haven't been super-explicit about documenting our React components.
{ children } | ||
</IconWrapper> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PaymentMethodUnavailablePill component lacks JSDoc documentation. Consider adding documentation that explains the component's purpose, its props (title and children), and how children are used as popover content.
/** | |
* PaymentMethodUnavailablePill | |
* | |
* Displays a styled pill indicating that a payment method is unavailable. | |
* Shows an info icon that, when hovered or clicked, displays a popover with additional information. | |
* | |
* @param {Object} props - Component props. | |
* @param {React.ReactNode} props.title - The text to display inside the pill. | |
* @param {React.ReactNode} props.children - The content to display inside the popover when the info icon is activated. | |
* @returns {JSX.Element} The rendered pill component. | |
*/ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, but happy to get feedback on this one. We haven't been super-explicit about documenting our React components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea consolidating the component 👍
client/components/payment-method-unavailable-due-conflict-pill/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify that clicking away from the popover hides the content.
I don't get this for the "Has plugin conflict" pill.
Screen.Recording.2025-08-29.at.1.22.25.PM.mov
@annemirasol, I tested this yesterday and it looks like this was a pre-existing issue, as I could reproduce the issue in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We can revisit the focusout issue when we've upgraded our libraries.
Changes proposed in this Pull Request:
While working on #4523, I noticed that we have duplicated all the styled elements and some implementation for the pill elements we show within the payment methods list. This PR consolidates those elements into a new
PaymentMethodUnavailablePill
component, as well as a secondaryPaymentMethodPopoverLink
element. This should mean we have consistent implementations and UX across pills, making it easier to improve the components in future, while also reducing our bundle size slightly.The new
PaymentMethodUnavailablePill
component accepts atitle
prop and all children will be displayed via thecontent
attribute of the deeperPopover
child element. ThePaymentMethodPopoverLink
component can be used for links within the popover content, as we want to consolidate the behaviour for those links as well. This prop is a wrapper around ana
element, but specifically defaults theonClick
,target
, andrel
props to simplify life for callers so they only need to specify thehref
to get the default behaviour.Testing instructions
git checkout try/refactor-payment-method-pills
npm run build
Requires currency
pill for payment methods that don't support the currencyChangelog entry
Changelog Entry Comment
Comment
Post merge