-
Notifications
You must be signed in to change notification settings - Fork 53
Add LearnHow link #2353
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
Add LearnHow link #2353
Conversation
📊 Performance Test ResultsComparing b0946c6 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
gcsecsey
left a comment
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.
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.
We removed the arrow and the associated aria-label for accessibility.
I'm not sure if it was intentional, so I wanted to flag it.
The learnHow boolean prop isn't very clear in my opinion. Consider using a label prop instead:
export function LearnMoreLink( {
docsLinksKey,
className,
label,
}: {
docsLinksKey: DocsLinkKey;
className?: string;
label?: string;
} ) {
const { __ } = useI18n();
const locale = useI18nLocale();
return (
<Button
className={ className }
onClick={ ( e: React.MouseEvent ) => {
e.stopPropagation();
getIpcApi().openURL( getLocalizedLink( locale, docsLinksKey ) );
} }
variant="link"
>
{ label ?? __( 'Learn more' ) }
</Button>
);
}If we want to restrict labels to specific variants, we could have an internal DocsLink component with two exported wrappers (LearnMoreLink and LearnHowLink).
It was intentional, the main reason is the same as for "Learn more" link - #2317 (comment)
I intentionally wanted to avoid custom labels, to avoid duplications (setting label={ __ ("Learn how") } ) and have consistency across Studio.
This sugesstion is great, thanks, I will go with it 👍 |
bcotrim
left a comment
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.
Thanks for making the changes @nightnei
LGTM 👍



Proposed Changes
Follow up to #2317
@katinthehatsite noticed that we don't have space for "Learn how" link. In the previous PR I didn't see this type of links, so this PR utilizes that component to avoid duplications and make that component reuzabled.
Testing Instructions
Apply the next diff: