Skip to content
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

Merged
merged 10 commits into from
Nov 9, 2023
Merged

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Nov 8, 2023

Description 📝

Add AGLB Feedback Link.

Before After
image image

How to test 🧪

Verification steps

(How to verify changes)

  • Navigate to http://localhost:3000/loadbalancers.
  • Verify Feedback link in all AGLB pages.
  • Verify LandingHeader Docs link alignment across the CM.
  • Link should navigate to google forms in a new tab.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@cpathipa cpathipa requested a review from a team as a code owner November 8, 2023 13:28
@cpathipa cpathipa requested review from jaalah-akamai and tyler-akamai and removed request for a team November 8, 2023 13:28
@cpathipa cpathipa marked this pull request as draft November 8, 2023 13:28
@cpathipa cpathipa self-assigned this Nov 8, 2023
@cpathipa cpathipa added the ACLB Relating to the Akamai Cloud Load Balancer label Nov 8, 2023
@cpathipa cpathipa marked this pull request as ready for review November 8, 2023 15:46
<Grid container sx={docsLink ? sxFeedbackLink : null}>
<Typography>
<Link external hideIcon={true} to={feedbackLink}>
<ExternalLinkIcon style={{ verticalAlign: 'text-top' }} />{' '}
Copy link
Contributor Author

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.

Copy link
Contributor

@abailly-akamai abailly-akamai Nov 9, 2023

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>
        )}

Screenshot 2023-11-09 at 09 33 43

Also, you don't really need a Typography around the Link.

@mjac0bs
Copy link
Contributor

mjac0bs commented Nov 8, 2023

@cpathipa - You might have already mentioned this to UX, but can we maintain consistent styling across our products in beta? VPC looks a little different in #9879.

Screenshot 2023-11-08 at 8 12 47 AM

@cpathipa
Copy link
Contributor Author

cpathipa commented Nov 8, 2023

@cpathipa - You might have already mentioned this to UX, but can we maintain consistent styling across our products in beta? VPC looks a little different in #9879.

Screenshot 2023-11-08 at 8 12 47 AM

@mjac0bs I'm not aware of this PR, I will make the Icon changes consistent with VPC feedback link.

<Grid container sx={docsLink ? sxFeedbackLink : null}>
<Typography>
<Link external hideIcon={true} to={feedbackLink}>
<ExternalLinkIcon style={{ verticalAlign: 'text-top' }} />{' '}
Copy link
Contributor

@abailly-akamai abailly-akamai Nov 9, 2023

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>
        )}

Screenshot 2023-11-09 at 09 33 43

Also, you don't really need a Typography around the Link.

@cpathipa
Copy link
Contributor Author

cpathipa commented Nov 9, 2023

@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.

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Nov 9, 2023

Verified:

  • Feedback link appears in all AGLB pages.
  • LandingHeader Docs link has proper alignment across CM
  • Link navigates to google forms in a new tab

@jaalah-akamai
Copy link
Contributor

Thanks for aligning this with our previous design

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed link styling is consistent with VPC BETA Feedback and the feedback link displays on the Landing, Details, and Create pages. ✅

Styling edge case:

Screenshots

Screenshot 2023-11-09 at 9 43 06 AM
Screenshot 2023-11-09 at 9 43 12 AM
Screenshot 2023-11-09 at 9 39 16 AM

Can we add some left margin for mobile view? When the link wraps on the smallest screens, it could be in line with the Breadcrumb.

@cpathipa
Copy link
Contributor Author

cpathipa commented Nov 9, 2023

@mjac0bs Added left margin for small screen. 33c441d

@cpathipa cpathipa merged commit 213cd70 into linode:develop Nov 9, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLB Relating to the Akamai Cloud Load Balancer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants