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.

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

feat: introduce as prop for Hyperlink #3082

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

Conversation

adamstankiewicz
Copy link
Member

@adamstankiewicz adamstankiewicz commented May 29, 2024

Description

Introduces as prop and types for Hyperlink to make it compatible with React Router's Link.

Note: May need to be updated after #3080 is reviewed/merged.

Deploy Preview

https://deploy-preview-3082--paragon-openedx.netlify.app/components/hyperlink

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.
  • 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.

Copy link

netlify bot commented May 29, 2024

Deploy Preview for paragon-openedx ready!

Name Link
🔨 Latest commit 9ead6aa
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/6657573524ba7f00088e2b55
😎 Deploy Preview https://deploy-preview-3082--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@adamstankiewicz adamstankiewicz marked this pull request as draft May 29, 2024 15:37
Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.17%. Comparing base (d4fce25) to head (9ead6aa).
Report is 1 commits behind head on master.

Files Patch % Lines
src/Hyperlink/index.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3082      +/-   ##
==========================================
- Coverage   93.19%   93.17%   -0.03%     
==========================================
  Files         249      249              
  Lines        4348     4349       +1     
  Branches     1034     1001      -33     
==========================================
  Hits         4052     4052              
- Misses        290      294       +4     
+ Partials        6        3       -3     

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

@adamstankiewicz adamstankiewicz force-pushed the ags/hyperlink-react-router-support branch from 7af05c8 to fa72a7d Compare May 29, 2024 16:22
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

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.

@adamstankiewicz adamstankiewicz force-pushed the ags/hyperlink-react-router-support branch from fa72a7d to 9ead6aa Compare May 29, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

[paragon-openedx.netlify.app] Hyperlink should be compatible with React Router Link
2 participants