Tech story [M3-6795]: Consolidate Link & ExternalLink #9411

abailly-akamai · 2023-07-17T20:58:01Z

Description 📝

One component to rule them all! 💍

This PR refactors the Link component so it can be used for all three use cases:

a relative URL, which will render a Link component from React Router.
an absolute, same domain/subdomain URL (ex: linode.com, akamai.com) that will render an a tag with target="_blank".
an absolute URL with the external prop, which will render an a tag with target="_blank" and an external link icon.

Preview 📷

Visual changes should only pertain to the presence (or not) of some external links. Remember that links opening in a new tab but directing a user to an Akamai or Linode domain/subdomain should not feature an external link icon

How to test 🧪

I can ask for reviewers to verify every link modified by this PR, however the areas to look for visual regressions or bad links should be:

Empty state landing pages
Docs Links on create pages (next to create buttons) (note - they are closer to the button now, which is intentional)
Help & Support area pages
Profile area
Storybook Link stories

abailly-akamai · 2023-07-19T17:22:39Z

packages/manager/src/App.tsx

-              Opens an external site in a new window
-            </span>
-          </div>
-          <GoTo open={goToOpen} onClose={() => setGoToOpen(false)} />


Because of this PR we do not need these IDs to refer our markup to for external links. This was not the cleanest way ^

abailly-akamai · 2023-07-19T17:24:41Z

packages/manager/src/components/CookieWarning.tsx

-        target="_blank"
-      >
-        :)
-      </a>


I don't want to crush any attempt at humor in our codebase, but this seems over the top 🤣

Humor crumbler! 🍪 No, agreed, this little easter egg has had it's run.

abailly-akamai · 2023-07-19T17:25:52Z

packages/manager/src/components/DocsLink/DocsLink.tsx

Modifying the DocsLink to use comply to our new API - also cleaned it up and replaced its .mdx story

abailly-akamai · 2023-07-19T17:28:00Z

packages/manager/src/components/EmptyLandingPageResources/ResourcesLinkIcon.tsx

Deleting this to not use yet another abstraction - this is for empty landing pages and we're making use of the new Link API now

abailly-akamai · 2023-07-19T17:29:46Z

packages/manager/src/components/ExternalLink/ExternalLink.stories.mdx

External Link shouldn't really be a stand alone now

abailly-akamai · 2023-07-19T17:58:18Z

.../manager/src/features/Profile/AuthenticationSettings/PhoneVerification/PhoneVerification.tsx

+                  onClick={onEdit}
+                >
+                  Edit
+                </LinkButton>


Small style adjustment I noticed was needed - unrelated to this refactor

Nice catch on the abnormally large 'Edit'.

The large edit button was in the mockups when we built the phone verification feature, but I'm cool with making it more normal. 😄

abailly-akamai · 2023-07-19T18:01:15Z

packages/manager/src/components/LinkButton.tsx

Unrelated to this PR. pretty sure this will go away as well eventually, but for now it is being used and I converted it to use the right component VS spreading styles

abailly-akamai · 2023-07-19T18:02:08Z

packages/manager/src/components/ExternalLink/ExternalLink.tsx

Goodbye ExternalLink, it's been a slice

abailly-akamai · 2023-07-19T18:03:05Z

packages/manager/src/components/EmptyLandingPageResources/ResourcesLinkIcon.tsx

Refactored the empty state landing pages to use the new API - we don't need yet another abstraction!

abailly-akamai · 2023-07-19T18:04:09Z

packages/manager/src/components/DocsLink/DocsLink.tsx

Refactored DocsLink to use the new API. No need to abstract that much. Also moved its story under component/Link since they are very much related

abailly-akamai · 2023-07-19T18:30:26Z

packages/manager/src/components/DocsLink/DocsLink.tsx

DocsLink has been refactored to use the new Link API instead of a previous button. Those should be links!

abailly-akamai · 2023-07-20T18:13:35Z

packages/manager/src/App.tsx

-        <span id="external-site-new-window">
-          Opens an external site in a new window
-        </span>
-      </div>


This is lo longer needed since we changed the API to no refer to those IDs

bnussman-akamai

Every link I could find looked good! Nice job on this... so much cleaner 🧼

mjac0bs

Thank you for all the work on this! It's exciting to have standardized our linking. I did a pretty thorough look at the diff and clicking around the areas you called out in the testing section. Approving, since I found no major issues - called out minor ones in comments.

I tried to keep an eye out for links that are currently external links and were previously ExternalLinks with hideIcon as a prop, because there's likely a UI reason we weren't displaying the icon and some styling is needed to make it better. I only caught a couple places where this was the place in OBJ (commented below); hopefully we don't come across others that I missed.

One other thing I found:

On http://localhost:3000/support/search/?query=test, there are links that are the wrong color in dark mode... and they're not actually links at all, but ListItems within the DocumentationResults component. These should really be Links. Mentioning it here, but it could be a follow up ticket if we want to stick with the current scope.

mjac0bs · 2023-07-20T20:15:39Z

packages/manager/src/components/CookieWarning.tsx

-        target="_blank"
-      >
-        :)
-      </a>


Humor crumbler! 🍪 No, agreed, this little easter egg has had it's run.

mjac0bs · 2023-07-20T21:13:33Z

packages/manager/src/features/Support/SupportTicketDetail/TabbedReply/MarkdownReference.tsx

-        </a>
+        {props.isReply ? 'reply' : 'question'}. For more examples see this{' '}
+        <Link external to="http://demo.showdownjs.com/">
+          markdown cheatsheet


I think this should still be capitalized since it's the name of a language

Suggested change

markdown cheatsheet

Markdown cheatsheet

mjac0bs · 2023-07-20T21:14:12Z

packages/manager/src/features/Support/Hively.tsx

TIL a Hively component exists!

mjac0bs · 2023-07-20T21:24:59Z

.../manager/src/features/Profile/AuthenticationSettings/PhoneVerification/PhoneVerification.tsx

+                  onClick={onEdit}
+                >
+                  Edit
+                </LinkButton>


Nice catch on the abnormally large 'Edit'.

mjac0bs · 2023-07-20T21:27:57Z

packages/manager/src/features/Profile/AuthenticationSettings/TPAProviders.tsx

+              <Link to={currentProvider.href}>
+                {`${currentProvider.displayName}` + ` website`}
+              </Link>


Suggested change

<Link to={currentProvider.href}>

{`${currentProvider.displayName}` + ` website`}

</Link>

<Link external to={currentProvider.href}>

{`${currentProvider.displayName}` + ` website`}

</Link>

Shouldn't this be considered an external link?

mjac0bs · 2023-07-20T21:36:31Z

packages/manager/src/features/ObjectStorage/BucketLanding/BucketDetailsDrawer.tsx

+            <Link external to={`https://${hostname}`}>
+              {truncateMiddle(hostname, 50)}
+            </Link>


If we show the external link icon here, I think we need some padding.

mjac0bs · 2023-07-20T21:41:41Z

packages/manager/src/features/ObjectStorage/BucketDetail/ObjectDetailsDrawer.tsx

+            <Link external to={url}>
+              {truncateMiddle(url, 50)}
+            </Link>


Same issue as BucketDetailsDrawer, I think.

mjac0bs · 2023-07-20T22:10:40Z

packages/manager/src/utilities/link.test.tsx

+        </div>
+      </>
+    );
+    expect(flattenChildrenIntoAriaLabel(children)).toBe('First Second Third');


Thanks for these tests. I was looking to understand if this function was doing what I thought.

mjac0bs · 2023-07-20T22:20:33Z

packages/manager/src/components/EmptyLandingPageResources/ResourcesLinksSubSection.tsx

Empty state landing looks better with white icons (in dark mode) and the docs arrow icon spacing change!

mjac0bs · 2023-07-20T22:28:44Z

packages/manager/src/components/Link.stories.tsx

- * This story is deprecated and will be removed in a future release.
- * Please use the `ExternalLink` component when you need to render an external link.
+ * An instance of Link component given an absolute URL, rendering an `a` tag with `target="_blank"`.
+ * This version provides an `external` prop, usually belonging to the same domain or a subdomain.<br />


Suggested change

* This version provides an `external` prop, usually belonging to the same domain or a subdomain.<br />

* This version provides an `external` prop, to be used when the site does not belong to the same domain or a subdomain.<br />

Is this what we mean here? Otherwise, I'm a little confused.

yes! you're right, fixed!

abailly-akamai · 2023-07-21T14:04:20Z

@bnussman-akamai @mjac0bs thanks so much for the reviews!

I addressed all feedback and also fixed the wrong markup on the search result page

abailly-akamai self-assigned this Jul 18, 2023

abailly-akamai force-pushed the tech-story-M3-6795 branch from c0b149a to eb7824c Compare July 19, 2023 17:20

abailly-akamai commented Jul 19, 2023

View reviewed changes

abailly-akamai force-pushed the tech-story-M3-6795 branch from 92c3b32 to 0606937 Compare July 19, 2023 18:10

abailly-akamai commented Jul 19, 2023

View reviewed changes

abailly-akamai marked this pull request as ready for review July 19, 2023 18:32

bnussman-akamai added Ready for Review Missing Changeset and removed Missing Changeset labels Jul 19, 2023

abailly-akamai force-pushed the tech-story-M3-6795 branch 3 times, most recently from a080f9a to 622373a Compare July 20, 2023 18:11

abailly-akamai commented Jul 20, 2023

View reviewed changes

bnussman-akamai approved these changes Jul 20, 2023

View reviewed changes

bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jul 20, 2023

mjac0bs approved these changes Jul 20, 2023

View reviewed changes

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jul 20, 2023

abailly-akamai force-pushed the tech-story-M3-6795 branch from 622373a to d8b71b6 Compare July 21, 2023 13:32

abailly-akamai added 7 commits July 21, 2023 17:14

Tech story: [M3-6795] initial commit

2bd5954

chore: [M3-6795] first pass at replacing all external links

814d69c

chore: [M3-6795] Consolidate instances of links

d616c9e

chore: [M3-6795] More consolidation

4bc4122

chore: [M3-6795] Docs Links

0d6342e

chore: [M3-6795] Cleanup

2840af9

chore: [M3-6795] Unit test

6de50de

abailly-akamai added 14 commits July 21, 2023 17:14

chore: [M3-6795] fix TPADialog unit test

6483e52

chore: [M3-6795] Fix e2e falied run

b6296cf

chore: [M3-6795] Cleanup and documentation

5e4288d

chore: [M3-6795] more clanup

d925e1c

chore: [M3-6795] DocsLink story

3924d19

chore: [M3-6795] DocsLink story - remove mdx

4356852

chore: [M3-6795] utils and testing

c13c1fa

chore: [M3-6795] story improvement

29480d7

chore: [M3-6795] moar cleanup

2d6ab3e

chore: [M3-6795] fix missing instance

be2de86

Added changeset: Refactor Link and deprecate ExternalLink

493dda3

chore: [M3-6795] improve e2e selector

36a4c0f

chore: [M3-6795] feedback 1

44ee4ca

chore: [M3-6795] feedback 2

c7d0583

abailly-akamai force-pushed the tech-story-M3-6795 branch from 6e2253f to c7d0583 Compare July 21, 2023 21:14

chore: [M3-6795] fix e2e tag

b057396

abailly-akamai merged commit e44928c into linode:develop Jul 23, 2023

This was referenced Aug 7, 2023

Release v1.99.0 - release → staging #9502

Merged

chore: [M3-6772] - Clean up one-off link styles #9526

Merged

bnussman-akamai mentioned this pull request Aug 15, 2023

fix: [M3-7024] - Fix Hively visual regression #9548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tech story [M3-6795]: Consolidate Link & ExternalLink #9411

Tech story [M3-6795]: Consolidate Link & ExternalLink #9411

abailly-akamai commented Jul 17, 2023 •

edited

Loading

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

mjac0bs Jul 20, 2023

bnussman-akamai Jul 20, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 19, 2023

abailly-akamai Jul 20, 2023

bnussman-akamai left a comment

mjac0bs left a comment

mjac0bs Jul 20, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 21, 2023

mjac0bs Jul 20, 2023

mjac0bs Jul 20, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 21, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 21, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 21, 2023

mjac0bs Jul 20, 2023

mjac0bs Jul 20, 2023

mjac0bs Jul 20, 2023

abailly-akamai Jul 21, 2023

abailly-akamai commented Jul 21, 2023

	* This version provides an `external` prop, usually belonging to the same domain or a subdomain.<br />
	* This version provides an `external` prop, to be used when the site does not belong to the same domain or a subdomain.<br />

Tech story [M3-6795]: Consolidate Link & ExternalLink #9411

Tech story [M3-6795]: Consolidate Link & ExternalLink #9411

Conversation

abailly-akamai commented Jul 17, 2023 • edited Loading

Description 📝

One component to rule them all! 💍

Preview 📷

How to test 🧪

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

mjac0bs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly-akamai commented Jul 21, 2023

abailly-akamai commented Jul 17, 2023 •

edited

Loading