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

feat: introduce as prop for Hyperlink #3082

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

feat: introduce as prop for Hyperlink #3082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97a577b
feat: introduce as prop for Hyperlink
adamstankiewicz May 29, 2024
9ead6aa
docs: add README example
adamstankiewicz May 29, 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
17 changes: 16 additions & 1 deletion src/Hyperlink/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ categories:
- Buttonlike
status: 'Needs Work'
designStatus: 'Done'
devStatus: 'To Do'
devStatus: 'Done'
notes: |
Improve prop naming. Deprecate content prop.
Use React.forwardRef for ref forwarding.
Expand Down Expand Up @@ -100,3 +100,18 @@ notes: |
</div>
</div>
```

## with custom link element

``Hyperlink`` typically relies on the standard HTML anchor tag (i.e., ``a``); however, this behavior may be overriden when the destination link is to an internal route where it should be using routing instead (e.g., ``Link`` from React Router).

```jsx live
<Hyperlink
as={GatsbyLink}
// `destination` is still a required prop even though the `to` takes precedence.
destination="/components/button"
to="/components/button"
>
Button
</Hyperlink>
```
18 changes: 14 additions & 4 deletions src/Hyperlink/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import {
type BsPrefixRefForwardingComponent as ComponentWithAsProp,
type BsPrefixProps,
} from 'react-bootstrap/esm/helpers';
import { Launch } from '../../icons';
import Icon from '../Icon';

export const HYPER_LINK_EXTERNAL_LINK_ALT_TEXT = 'in a new tab';
export const HYPER_LINK_EXTERNAL_LINK_TITLE = 'Opens in a new tab';

interface Props extends Omit<React.ComponentPropsWithRef<'a'>, 'href' | 'target'> {
interface HyperlinkProps extends BsPrefixProps, Omit<React.ComponentPropsWithRef<'a'>, 'href' | 'target'> {
/** specifies the URL */
destination: string;
Copy link
Member Author

@adamstankiewicz adamstankiewicz May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When using Hyperlink with Link from React Router (or GatsbyLink on the Paragon docs site example), the destination prop will not suffice as it gets passed to an href prop, leading to the link getting treated as an external URL (triggering full-page refresh).

Both Link and GatsbyLink utilize the standard hyperlink (i.e., a) when href prop is provided; to get it to work with internal routing, the to prop must be passed.

Because we use prop spreading of all attributes passed to the Hyperlink component, consumers can pass to prop but would continue to need to pass destination as it's a required prop.

We will need to determine how consumers can pass to without needing to also pass an identical value to destination (i.e., href), e.g.:

<Hyperlink
  as={GatsbyLink}
  to="/components/button"
  // `destination` is still a required prop even though the `to` takes precedence.
  destination="/components/button"
>
  Button
</Hyperlink>

Copy link
Member Author

Choose a reason for hiding this comment

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

@bradenmacdonald Curious if you might have any thoughts on how to mitigate this concern around to vs. href vs. destination?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was wondering about this.

I think one nice option is to change the Paragon hyperlink to accept to instead of destination and just treat destination as a deprecated alias for to ?

Otherwise to avoid the need to introspect as and determine what prop to pass based on some conditions, you could change the logic that currently converts destination to href and instead convert destination to both href and to, and unconditionally pass both props to the inner as component. Though I think this may display a DOM warning in the console that a doesn't accept a to attribute. So maybe some conditional logic is actually needed.

/** Content of the hyperlink */
Expand All @@ -27,7 +31,10 @@ interface Props extends Omit<React.ComponentPropsWithRef<'a'>, 'href' | 'target'
target?: '_blank' | '_self';
}

const Hyperlink = React.forwardRef<HTMLAnchorElement, Props>(({
type HyperlinkType = ComponentWithAsProp<'a', HyperlinkProps>;

const Hyperlink: HyperlinkType = React.forwardRef<HTMLAnchorElement, HyperlinkProps>(({
as: Component = 'a',
className,
destination,
children,
Expand Down Expand Up @@ -77,7 +84,7 @@ const Hyperlink = React.forwardRef<HTMLAnchorElement, Props>(({
}

return (
<a
<Component
ref={ref}
className={classNames(
'pgn__hyperlink',
Expand All @@ -95,11 +102,12 @@ const Hyperlink = React.forwardRef<HTMLAnchorElement, Props>(({
>
{children}
{externalLinkIcon}
</a>
</Component>
);
});

Hyperlink.defaultProps = {
as: 'a',
className: undefined,
target: '_self',
onClick: () => {},
Expand All @@ -111,6 +119,8 @@ Hyperlink.defaultProps = {
};

Hyperlink.propTypes = {
/** specifies the component element type to render for the hyperlink */
as: PropTypes.elementType,
/** specifies the URL */
destination: PropTypes.string.isRequired,
/** Content of the hyperlink */
Expand Down
Loading