-
Notifications
You must be signed in to change notification settings - Fork 69
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 TypeScript types for <Hyperlink> #3077
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,45 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import classNames from 'classnames'; | ||
import isRequiredIf from 'react-proptype-conditional-require'; | ||
import { Launch } from '../../icons'; | ||
import Icon from '../Icon'; | ||
|
||
import withDeprecatedProps, { DeprTypes } from '../withDeprecatedProps'; | ||
|
||
export const HYPER_LINK_EXTERNAL_LINK_ALT_TEXT = 'in a new tab'; | ||
export const HYPER_LINK_EXTERNAL_LINK_TITLE = 'Opens in a new tab'; | ||
|
||
const Hyperlink = React.forwardRef((props, ref) => { | ||
const { | ||
className, | ||
destination, | ||
children, | ||
target, | ||
onClick, | ||
externalLinkAlternativeText, | ||
externalLinkTitle, | ||
variant, | ||
isInline, | ||
showLaunchIcon, | ||
...attrs | ||
} = props; | ||
interface Props extends Omit<React.ComponentPropsWithoutRef<'a'>, 'href' | 'target'> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [clarification] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! In this case, the |
||
/** specifies the URL */ | ||
destination: string; | ||
/** Content of the hyperlink */ | ||
children: React.ReactNode; | ||
/** 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left this as an optional prop. There was propTypes code below to make If we want to make those props conditionally required (when target= We probably should make that required at some point, because the default value ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, you're likely correct the
Yes, this is usually the case. See the recently filed issue #3050 regarding adapting
Paragon has had some i18n support in its components for a couple years now (see ADR and the README), except IIRC the It does seem by moving translations to If we want it to have a translated default prop (I agree, we should; looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
/** Tooltip text for the "opens in new tab" icon, if target="_blank". e.g. _("Opens in a new tab"). */ | ||
externalLinkTitle?: string; | ||
/** type of hyperlink */ | ||
variant?: 'default' | 'muted' | 'brand'; | ||
/** Display the link with an underline. By default, it is only underlined on hover. */ | ||
isInline?: boolean; | ||
/** specify if we need to show launch Icon. By default, it will be visible. */ | ||
showLaunchIcon?: boolean; | ||
target?: '_blank' | '_self'; | ||
} | ||
|
||
const Hyperlink = React.forwardRef<HTMLAnchorElement, Props>(({ | ||
className, | ||
destination, | ||
children, | ||
target, | ||
onClick, | ||
externalLinkAlternativeText, | ||
externalLinkTitle, | ||
variant, | ||
isInline, | ||
showLaunchIcon, | ||
...attrs | ||
}, ref) => { | ||
let externalLinkIcon; | ||
|
||
if (target === '_blank') { | ||
|
@@ -105,32 +121,20 @@ Hyperlink.propTypes = { | |
* loaded into the same browsing context as the current one. | ||
* If the target is `_blank` (opening a new window) `rel='noopener'` will be added to the anchor tag to prevent | ||
* any potential [reverse tabnabbing attack](https://www.owasp.org/index.php/Reverse_Tabnabbing). | ||
*/ | ||
target: PropTypes.string, | ||
*/ | ||
target: PropTypes.oneOf(['_blank', '_self']), | ||
/** specifies the callback function when the link is clicked */ | ||
onClick: PropTypes.func, | ||
/** specifies the text for links with a `_blank` target (which loads the URL in a new browsing context). */ | ||
externalLinkAlternativeText: isRequiredIf( | ||
PropTypes.string, | ||
props => props.target === '_blank', | ||
), | ||
/** specifies the title for links with a `_blank` target (which loads the URL in a new browsing context). */ | ||
externalLinkTitle: isRequiredIf( | ||
PropTypes.string, | ||
props => props.target === '_blank', | ||
), | ||
/** Alt text for the icon indicating that this link opens in a new tab, if target="_blank". e.g. _("in a new tab") */ | ||
externalLinkAlternativeText: PropTypes.string, | ||
/** Tooltip text for the "opens in new tab" icon, if target="_blank". e.g. _("Opens in a new tab"). */ | ||
externalLinkTitle: PropTypes.string, | ||
/** type of hyperlink */ | ||
variant: PropTypes.oneOf(['default', 'muted', 'brand']), | ||
/** specify the link style. By default, it will be underlined. */ | ||
/** Display the link with an underline. By default, it is only underlined on hover. */ | ||
isInline: PropTypes.bool, | ||
/** specify if we need to show launch Icon. By default, it will be visible. */ | ||
showLaunchIcon: PropTypes.bool, | ||
}; | ||
|
||
export default withDeprecatedProps(Hyperlink, 'Hyperlink', { | ||
/** specifies the text or element that a URL should be associated with */ | ||
content: { | ||
deprType: DeprTypes.MOVED, | ||
newName: 'children', | ||
}, | ||
Comment on lines
-132
to
-135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 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 |
||
}); | ||
export default Hyperlink; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
/* eslint-disable import/no-extraneous-dependencies */ | ||
import 'regenerator-runtime/runtime'; | ||
|
||
import '@testing-library/jest-dom'; | ||
|
@@ -20,6 +21,6 @@ class ResizeObserver { | |
|
||
window.ResizeObserver = ResizeObserver; | ||
|
||
window.crypto = { | ||
getRandomValues: arr => crypto.randomBytes(arr.length), | ||
(window as any).crypto = { | ||
getRandomValues: (arr: any) => crypto.randomBytes(arr.length), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adamstankiewicz I see you added this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}; |
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.
nit: could opt for
jest.clearAllMocks
vs. clearing the specificonClick
mock (even though it's the only mock here).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.
[inform] When I open my PR to add support for
Link
usage withHyperlink
, I will plan to make this change (as well as remove thewindow.crypto
stuff insetupTest.js
per the other comment).