-
Notifications
You must be signed in to change notification settings - Fork 367
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: [M3-7358] - Add AGLB Feedback Link #9885
Conversation
<Grid container sx={docsLink ? sxFeedbackLink : null}> | ||
<Typography> | ||
<Link external hideIcon={true} to={feedbackLink}> | ||
<ExternalLinkIcon style={{ verticalAlign: 'text-top' }} />{' '} |
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.
IMO, ExternalLinkIcon makes more sense here. Since its navigating users from CM. But, needs confirmation from UX.
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 already have an external link icon on the component but you hide it to add another one. why not just use our component treatment? (or the proper docs link treatment)
{feedbackLink && (
<Grid container sx={docsLink ? sxFeedbackLink : null}>
<Link external to={feedbackLink}>
{feedbackLinkLabel}
</Link>
</Grid>
)}
Also, you don't really need a Typography around the Link.
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerBasicCreate.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerCreate.tsx
Outdated
Show resolved
Hide resolved
<Grid container sx={docsLink ? sxFeedbackLink : null}> | ||
<Typography> | ||
<Link external hideIcon={true} to={feedbackLink}> | ||
<ExternalLinkIcon style={{ verticalAlign: 'text-top' }} />{' '} |
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 already have an external link icon on the component but you hide it to add another one. why not just use our component treatment? (or the proper docs link treatment)
{feedbackLink && (
<Grid container sx={docsLink ? sxFeedbackLink : null}>
<Link external to={feedbackLink}>
{feedbackLinkLabel}
</Link>
</Grid>
)}
Also, you don't really need a Typography around the Link.
@abailly-akamai My initial thought was to maintain a separation of concerns, which is why I chose to implement a separate code block for the feedback component. This decision resulted in hiding the default icon that appears after the link text. However, our requirement was to display the icon before the link text. Additionally, I was not aware of #9879 that might have addressed this. |
Verified:
|
Thanks for aligning this with our previous design |
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.
Description 📝
Add AGLB Feedback Link.
How to test 🧪
Verification steps
(How to verify changes)
LandingHeader
Docs link alignment across the CM.As an Author I have considered 🤔
Check all that apply