-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
feat: Add link to pending transaction alert #28721
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
5cbeb48
to
363b9d8
Compare
d3c433e
to
e168314
Compare
Builds ready [a272f3d]
Page Load Metrics (1859 ± 88 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Hey Pedro let's not have a hover state for this. Let's use primary/ defualt for the line below to indicate it's clickable. Otherwise it looks good to me. Thanks!
|
Builds ready [490939f]
Page Load Metrics (1946 ± 57 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
Hey @pedronfigueiredo, one question—could we also check the HTML that is rendered? I want to ensure we aren't rendering <p>
within <p>
and that the HTML is semantically correct. Also, could you fill out the PR description so I can better understand what this PR entails? 🙏
@@ -31,7 +31,7 @@ export interface BannerBaseStyleUtilityProps extends StyleUtilityProps { | |||
/** | |||
* The description is the content area below BannerBase title | |||
*/ | |||
description?: string; | |||
description?: string | React.ReactNode; |
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 description is wrapped by p
tag what are we passing here that requires a node? The banners should also accept a children
prop which might be more suited for a custom node.
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
<Text color={TextColor.primaryDefault}> | ||
<a | ||
href={ZENDESK_URLS.SPEEDUP_CANCEL} | ||
key="link" | ||
target="_blank" | ||
rel="noreferrer noopener" | ||
> | ||
{t('pendingTransactionAlertMessageHyperlink')} | ||
</a> |
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.
non-blocking: If you wanted to use a design system component here for consistency you could use
import { ButtonLink } from '../../../../../components/component-library';
and
<Text color={TextColor.primaryDefault}> | |
<a | |
href={ZENDESK_URLS.SPEEDUP_CANCEL} | |
key="link" | |
target="_blank" | |
rel="noreferrer noopener" | |
> | |
{t('pendingTransactionAlertMessageHyperlink')} | |
</a> | |
<ButtonLink | |
href={ZENDESK_URLS.SPEEDUP_CANCEL} | |
key="link" | |
target="_blank" | |
rel="noreferrer noopener" | |
> | |
{t('pendingTransactionAlertMessageHyperlink')} | |
</ButtonLink> |
68c7534
to
be44db1
Compare
Builds ready [be44db1]
Page Load Metrics (1901 ± 224 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
be44db1
to
ab65ccf
Compare
286dd74
ab65ccf
to
286dd74
Compare
Builds ready [286dd74]
Page Load Metrics (1859 ± 61 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR substitutes a purely text based alert for pending transactions with one that includes a hyperlink to the docs.
Related issues
Fixes: #28308
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist