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.

Sign up for GitHub

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

Jump to bottom

Add TypeScript types for <Hyperlink> #3077

Merged
merged 3 commits into from
May 22, 2024
Merged

Add TypeScript types for <Hyperlink> #3077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
affa288
chore: convert setupTest to TypeScript
bradenmacdonald May 17, 2024
35b9573
feat!: add typings for <Hyperlink>, remove long-deprecated 'content' …
bradenmacdonald May 17, 2024
c9c030d
fix: use ComponentPropsWithRef since <Hyperlink> does accept a ref
bradenmacdonald May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ module.exports = {
{
devDependencies: [
'**/*.stories.jsx',
'src/setupTest.js',
'src/setupTest.ts',
'**/*.test.jsx',
'**/*.test.js',
'config/*.js',
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@
"^.+\\.tsx?$": "ts-jest"
},
"setupFilesAfterEnv": [
"./src/setupTest.js"
"./src/setupTest.ts"
],
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/__mocks__/fileMock.js",
Expand All @@ -164,7 +164,7 @@
],
"coveragePathIgnorePatterns": [
"/node_modules/",
"src/setupTest.js",
"src/setupTest.ts",
"src/index.js",
"/tests/",
"/www/",
Expand Down
31 changes: 21 additions & 10 deletions src/Hyperlink/Hyperlink.test.jsx → src/Hyperlink/Hyperlink.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,34 @@ import userEvent from '@testing-library/user-event';

import Hyperlink from '.';

const content = 'content';
const destination = 'destination';
const content = 'content';
const onClick = jest.fn();
const props = {
content,
destination,
onClick,
};
const externalLinkAlternativeText = 'externalLinkAlternativeText';
const externalLinkTitle = 'externalLinkTitle';
const externalLinkProps = {
target: '_blank',
target: '_blank' as const,
externalLinkAlternativeText,
externalLinkTitle,
...props,
};

describe('correct rendering', () => {
beforeEach(() => {
onClick.mockClear();
Copy link
Member

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 specific onClick mock (even though it's the only mock here).

Copy link
Member

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 with Hyperlink, I will plan to make this change (as well as remove the window.crypto stuff in setupTest.js per the other comment).

});

it('renders Hyperlink', async () => {
const { getByRole } = render(<Hyperlink {...props} />);
const { getByRole } = render(<Hyperlink {...props}>{content}</Hyperlink>);
const wrapper = getByRole('link');
expect(wrapper).toBeInTheDocument();

expect(wrapper).toHaveClass('pgn__hyperlink');
expect(wrapper).toHaveClass('standalone-link');
expect(wrapper).toHaveTextContent(content);
expect(wrapper).toHaveAttribute('href', destination);
expect(wrapper).toHaveAttribute('target', '_self');
Expand All @@ -36,8 +40,17 @@ describe('correct rendering', () => {
expect(onClick).toHaveBeenCalledTimes(1);
});

it('renders an underlined Hyperlink', async () => {
const { getByRole } = render(<Hyperlink isInline {...props}>{content}</Hyperlink>);
const wrapper = getByRole('link');
expect(wrapper).toBeInTheDocument();
expect(wrapper).toHaveClass('pgn__hyperlink');
expect(wrapper).not.toHaveClass('standalone-link');
expect(wrapper).toHaveClass('inline-link');
});

it('renders external Hyperlink', () => {
const { getByRole, getByTestId } = render(<Hyperlink {...externalLinkProps} />);
const { getByRole, getByTestId } = render(<Hyperlink {...externalLinkProps}>{content}</Hyperlink>);
const wrapper = getByRole('link');
const icon = getByTestId('hyperlink-icon');
const iconSvg = icon.querySelector('svg');
Expand All @@ -53,18 +66,16 @@ describe('correct rendering', () => {

describe('security', () => {
it('prevents reverse tabnabbing for links with target="_blank"', () => {
const { getByRole } = render(<Hyperlink {...externalLinkProps} />);
const { getByRole } = render(<Hyperlink {...externalLinkProps}>{content}</Hyperlink>);
const wrapper = getByRole('link');
expect(wrapper).toHaveAttribute('rel', 'noopener noreferrer');
});
});

describe('event handlers are triggered correctly', () => {
let spy;
beforeEach(() => { spy = jest.fn(); });

it('should fire onClick', async () => {
const { getByRole } = render(<Hyperlink {...props} onClick={spy} />);
const spy = jest.fn();
const { getByRole } = render(<Hyperlink {...props} onClick={spy}>{content}</Hyperlink>);
const wrapper = getByRole('link');
expect(spy).toHaveBeenCalledTimes(0);
await userEvent.click(wrapper);
Expand Down
78 changes: 41 additions & 37 deletions src/Hyperlink/index.jsx → src/Hyperlink/index.tsx
View file
Open in desktop
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.ComponentPropsWithRef<'a'>, 'href' | 'target'> {
/** 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;
Copy link
Contributor Author

@bradenmacdonald bradenmacdonald May 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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?

Copy link
Member

@adamstankiewicz adamstankiewicz May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Link :)

/** 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') {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 alpha design tokens release, too.

});
export default Hyperlink;
2 changes: 1 addition & 1 deletion src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
export { default as Bubble } from './Bubble';
export { default as Chip, CHIP_PGN_CLASS } from './Chip';
export { default as ChipCarousel } from './ChipCarousel';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
export { default as Icon } from './Icon';

// // // // // // // // // // // // // // // // // // // // // // // // // // //
Expand Down Expand Up @@ -72,7 +73,6 @@ export const
FormAutosuggestOption: any,
InputGroup: any;
// from './Form';
export const Hyperlink: any, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT: string, HYPER_LINK_EXTERNAL_LINK_TITLE: string; // from './Hyperlink';
export const IconButton: any, IconButtonWithTooltip: any; // from './IconButton';
export const IconButtonToggle: any; // from './IconButtonToggle';
export const Input: any; // from './Input';
Expand Down
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
export { default as Bubble } from './Bubble';
export { default as Chip, CHIP_PGN_CLASS } from './Chip';
export { default as ChipCarousel } from './ChipCarousel';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
export { default as Icon } from './Icon';

// // // // // // // // // // // // // // // // // // // // // // // // // // //
Expand Down Expand Up @@ -72,7 +73,6 @@ export {
FormAutosuggestOption,
InputGroup,
} from './Form';
export { default as Hyperlink, HYPER_LINK_EXTERNAL_LINK_ALT_TEXT, HYPER_LINK_EXTERNAL_LINK_TITLE } from './Hyperlink';
export { default as IconButton, IconButtonWithTooltip } from './IconButton';
export { default as IconButtonToggle } from './IconButtonToggle';
export { default as Input } from './Input';
Expand Down
5 changes: 3 additions & 2 deletions src/setupTest.js → src/setupTest.ts
View file
Open in desktop
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';
Expand All @@ -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),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@adamstankiewicz adamstankiewicz May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 alpha release, too (source) 😄

};
Loading