Add TypeScript types for <Hyperlink> #3077

bradenmacdonald · 2024-05-17T21:50:21Z

Description

Working to incrementally expand typing in paragon. This adds types for <Hyperlink>

I removed the content prop which has been deprecated for five years, since v4.0.2.

I also clarified the description of props like externalLinkAlternativeText and isInline which were very unclear about what they were doing.

Deploy Preview

Will be available later

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable. n/a
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please request an a11y review for the PR in the #wg-paragon Open edX Slack channel.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

…prop

openedx-webhooks · 2024-05-17T21:50:25Z

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

netlify · 2024-05-17T21:50:39Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`c9c030d`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/664d1682b04d350007f391b0
😎 Deploy Preview	https://deploy-preview-3077--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-05-17T22:01:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.19%. Comparing base (9993fb3) to head (c9c030d).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3077   +/-   ##
=======================================
  Coverage   93.18%   93.19%           
=======================================
  Files         249      249           
  Lines        4342     4348    +6     
  Branches     1000     1000           
=======================================
+ Hits         4046     4052    +6     
  Misses        293      293           
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bradenmacdonald · 2024-05-18T02:17:12Z

src/Hyperlink/index.tsx

+  /** Custom class names for the hyperlink */
+  className?: string;
+  /** Alt text for the icon indicating that this link opens in a new tab, if target="_blank". e.g. _("in a new tab") */
+  externalLinkAlternativeText?: string;


I left this as an optional prop. There was propTypes code below to make externalLinkAlternativeText required when target="_blank" but this wasn't actually working as far as I can tell, especially since a default value is given.

If we want to make those props conditionally required (when target=_blank), I can express that in the TypeScript types, but there are several instances in this repo alone that do not do that (example) and I suspect it would then show type errors in many other MFEs as well (if MFEs even use this component? Maybe they use react-router's Link instead).

We probably should make that required at some point, because the default value (HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, above) is not internationalized. But the only good way to do that that I could think of is to require each MFE to subclass Hyperlink with a component that provides internationalized defaults for these fields, and that seems like too much work. Is there not some way to provide internationalized defaults in Paragon itself?

but this wasn't actually working as far as I can tell, especially since a default value is given.

Yeah, you're likely correct the ifRequiredWhen isn't really doing much due to the default value.

I suspect it would then show type errors in many other MFEs as well (if MFEs even use this component? Maybe they use react-router's Link instead).

Yes, this is usually the case. See the recently filed issue #3050 regarding adapting Hyperlink to be compatible with Link, such that Link usage can have more consistent styling.

We probably should make that required at some point, because the default value (HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, above) is not internationalized. But the only good way to do that that I could think of is to require each MFE to subclass Hyperlink with a component that provides internationalized defaults for these fields, and that seems like too much work. Is there not some way to provide internationalized defaults in Paragon itself?

Paragon has had some i18n support in its components for a couple years now (see ADR and the README), except IIRC the pull_translations job routinely/currently fails due to an translation format error of some kind that's prevented the translations from updating in awhile. Related, I'm also not sure if Paragon has been migrated to using openedx-atlas / openedx-translations yet (unlikely), or if it intends to; @brian-smith-tcril might be able to share some insight about any plans to migrate.

It does seem by moving translations to openedx-translations, most MFEs are no longer importing/supplying paragonMessages anymore (e.g., removed from frontend-app-learning in this commit; I'm personally not sure if the Paragon i18n messages (and the shared header/footer component i18n messages) are somehow still passed to the MFEs, or if that support was (intentionally?) removed as part of the migration to openedx-translations?

If we want it to have a translated default prop (I agree, we should; looks like Hyperlink was missed in the original introduction of i18n for hardcoded English strings), then ideally we mark the strings for i18n via react-intl's defineMessage / intl.formatMessage or FormattedMessage, similar to a handful of other components in Paragon today; if the prop is provided, it gets used (where consumer might pass its own i18n string), otherwise falling back to the default i18n Paragon message.

Thanks for the context on i18n! I'm glad to see there is something in place, at least partially. I can look at fixing that in a follow-up PR.

Also great to see there's a plan to support Link :)

brian-smith-tcril · 2024-05-20T22:47:06Z

@adamstankiewicz I'd like to merge this but I don't want to get in the way of #3050

Would this cause any problems with the work being done towards that?

adamstankiewicz · 2024-05-21T02:11:54Z

src/Hyperlink/index.tsx

-  content: {
-    deprType: DeprTypes.MOVED,
-    newName: 'children',
-  },


🎉 great to clean this up :) related to removing deprecated stuff, I'm looking forward to having all the deprecated UI components removed as part of the alpha design tokens release, too.

adamstankiewicz · 2024-05-21T02:24:07Z

src/Hyperlink/index.tsx

-    showLaunchIcon,
-    ...attrs
-  } = props;
+interface Props extends Omit<React.ComponentPropsWithoutRef<'a'>, 'href' | 'target'> {


[clarification] Hyperlink uses forwardRef below, yet the Props interface is defining it as React.ComponentPropsWithoutRef; should it be using ComponentPropsWithRef?

Good question! In this case, the forwardRef wrapper is overriding the type and injecting the ref prop anyways, so it doesn't really matter whether we use WithoutRef or WithRef here - the type that will be seen elsewhere in the code is the same. But I think it makes sense to use withRef since we are dealing with refs, and it's perhaps less confusing that way. So I just pushed a small commit to change it.

adamstankiewicz · 2024-05-21T02:27:30Z

@adamstankiewicz I'd like to merge this but I don't want to get in the way of #3050

Would this cause any problems with the work being done towards that?

I hadn't quite gotten to creating that PR last week; still hope to this week. This work shouldn't cause any problems. The fix for the #3050 is additive (i.e., would introduce a new as prop), where if this PR merges first, the PR for #3050 would ensure the as prop is appropriately typed.

Added a couple other review comments onto the PR.

bradenmacdonald · 2024-05-21T22:07:58Z

src/setupTest.ts

-window.crypto = {
-  getRandomValues: arr => crypto.randomBytes(arr.length),
+(window as any).crypto = {
+  getRandomValues: (arr: any) => crypto.randomBytes(arr.length),


@adamstankiewicz I see you added this getRandomValues stub in eac3925 , but I noticed just now that the test suite passes just fine if I completely remove this line. Is it still necessary/useful?

Huh, looked back through the PR that added this bit; I'm honestly not sure why it was added at this point! Probably can be safely removed. Actually, it looks like it's already been removed in Paragon's alpha release, too (source) 😄

bradenmacdonald · 2024-05-21T22:08:43Z

Thanks for the quick reviews @brian-smith-tcril and @adamstankiewicz ! Could one of you please merge this at your convenience? :)

adamstankiewicz · 2024-05-22T12:54:56Z

src/Hyperlink/Hyperlink.test.tsx

  externalLinkAlternativeText,
  externalLinkTitle,
  ...props,
 };

 describe('correct rendering', () => {
+  beforeEach(() => {
+    onClick.mockClear();


nit: could opt for jest.clearAllMocks vs. clearing the specific onClick mock (even though it's the only mock here).

[inform] When I open my PR to add support for Link usage with Hyperlink, I will plan to make this change (as well as remove the window.crypto stuff in setupTest.js per the other comment).

openedx-webhooks · 2024-05-22T13:00:18Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-semantic-release-bot · 2024-05-22T13:10:25Z

🎉 This PR is included in version 22.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

bradenmacdonald added 2 commits May 17, 2024 14:45

chore: convert setupTest to TypeScript

affa288

feat!: add typings for <Hyperlink>, remove long-deprecated 'content' …

35b9573

…prop

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 17, 2024

bradenmacdonald force-pushed the type-hyperlink branch from bd92ad3 to 35b9573 Compare May 17, 2024 21:50

bradenmacdonald commented May 18, 2024

View reviewed changes

bradenmacdonald requested a review from brian-smith-tcril May 18, 2024 02:18

brian-smith-tcril approved these changes May 20, 2024

View reviewed changes

adamstankiewicz reviewed May 21, 2024

View reviewed changes

fix: use ComponentPropsWithRef since <Hyperlink> does accept a ref

c9c030d

bradenmacdonald commented May 21, 2024

View reviewed changes

adamstankiewicz approved these changes May 22, 2024

View reviewed changes

adamstankiewicz merged commit d4fce25 into openedx:master May 22, 2024
10 checks passed

openedx-semantic-release-bot added the released label May 22, 2024

PKulkoRaccoonGang pushed a commit that referenced this pull request Jul 7, 2024

feat: Add TypeScript types for <Hyperlink> (#3077)

2313f30

PKulkoRaccoonGang pushed a commit that referenced this pull request Jul 22, 2024

feat: Add TypeScript types for <Hyperlink> (#3077)

2d71ab5

PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 4, 2024

feat: Add TypeScript types for <Hyperlink> (#3077)

aaed46f

PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 5, 2024

feat: Add TypeScript types for <Hyperlink> (#3077)

2b95727

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript types for <Hyperlink> #3077

Add TypeScript types for <Hyperlink> #3077

bradenmacdonald commented May 17, 2024 •

edited

Loading

openedx-webhooks commented May 17, 2024

netlify bot commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

bradenmacdonald May 18, 2024 •

edited

Loading

adamstankiewicz May 21, 2024 •

edited

Loading

bradenmacdonald May 21, 2024

brian-smith-tcril commented May 20, 2024

adamstankiewicz May 21, 2024

adamstankiewicz May 21, 2024

bradenmacdonald May 21, 2024

adamstankiewicz commented May 21, 2024

bradenmacdonald May 21, 2024

adamstankiewicz May 22, 2024 •

edited

Loading

bradenmacdonald commented May 21, 2024

adamstankiewicz May 22, 2024

adamstankiewicz May 22, 2024

openedx-webhooks commented May 22, 2024

openedx-semantic-release-bot commented May 22, 2024

Add TypeScript types for <Hyperlink> #3077

Add TypeScript types for <Hyperlink> #3077

Conversation

bradenmacdonald commented May 17, 2024 • edited Loading

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

openedx-webhooks commented May 17, 2024

netlify bot commented May 17, 2024 • edited Loading

✅ Deploy Preview for paragon-openedx ready!

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

bradenmacdonald May 18, 2024 • edited Loading

Choose a reason for hiding this comment

adamstankiewicz May 21, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald May 21, 2024

Choose a reason for hiding this comment

brian-smith-tcril commented May 20, 2024

adamstankiewicz May 21, 2024

Choose a reason for hiding this comment

adamstankiewicz May 21, 2024

Choose a reason for hiding this comment

bradenmacdonald May 21, 2024

Choose a reason for hiding this comment

adamstankiewicz commented May 21, 2024

bradenmacdonald May 21, 2024

Choose a reason for hiding this comment

adamstankiewicz May 22, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald commented May 21, 2024

adamstankiewicz May 22, 2024

Choose a reason for hiding this comment

adamstankiewicz May 22, 2024

Choose a reason for hiding this comment

openedx-webhooks commented May 22, 2024

openedx-semantic-release-bot commented May 22, 2024

bradenmacdonald commented May 17, 2024 •

edited

Loading

netlify bot commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

bradenmacdonald May 18, 2024 •

edited

Loading

adamstankiewicz May 21, 2024 •

edited

Loading

adamstankiewicz May 22, 2024 •

edited

Loading