Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

rewrite "external" urls pointing to the current domain into internal urls #719

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jan 23, 2024

As we are reusing comments between source code and docs, our linked urls are absolute (otherwise they couldn't be used from the IDE), not relative.

The current isExternal check recognizes these links to the docs as "external" and opens them in a new tab - this PR aims to fix that.

See apollographql/apollo-client#11381 (comment)

This now rewrites urls into domain-absolute urls that will correctly be picked up by the GatsbyLink component for internal documentation.

See the deploy preview at https://65b114d41ee09600089b6792--apollo-monodocs.netlify.app/react/api/core/ApolloClient/#ApolloClientOptions

@phryneas phryneas requested a review from jerelmiller January 23, 2024 13:52
@phryneas phryneas requested a review from a team as a code owner January 23, 2024 13:52
Copy link

netlify bot commented Jan 23, 2024

Deploy Preview for apollo-monodocs ready!

Name Link
🔨 Latest commit 2c3492c
🔍 Latest deploy log https://app.netlify.com/sites/apollo-monodocs/deploys/65b9748738d8bd00082cdcfa
😎 Deploy Preview https://deploy-preview-719--apollo-monodocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 35 (🔴 down 39 from production)
Accessibility: 85 (no change from production)
Best Practices: 92 (no change from production)
SEO: 95 (🟢 up 8 from production)
PWA: 50 (🟢 up 20 from production)
View the detailed breakdown and full score reports

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

@@ -7,7 +7,13 @@ import {withPrefix} from 'gatsby';
export const NavContext = createContext();
export const PathContext = createContext();

export const isUrl = string => /^https?:\/\//.test(string);
const urlRegex = /^https?:\/\//;
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved those out into constants to the regex is not recreated for every call and can be optimized by the engine.

@phryneas
Copy link
Member Author

I've double-checked this and it will need another fix - see https://deploy-preview-719--apollo-monodocs.netlify.app/react/api/core/ApolloClient#ApolloClientOptions

image

This link currently ends up as /react/api/core/https:/www.apollographql.com/docs/react/caching/cache-configuration

@phryneas phryneas force-pushed the pr/externalUrlCheck branch from 05ee41b to f59f877 Compare January 24, 2024 13:46
@phryneas phryneas changed the title check "external" urls more thoroughly rewrite "external" urls pointing to the current domain into internal urls Jan 24, 2024
@phryneas
Copy link
Member Author

Okay, fixed up and requesting a re-review :)

@phryneas phryneas requested a review from jerelmiller January 24, 2024 13:57
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

🎉

@jerelmiller jerelmiller merged commit c581d57 into main Jan 30, 2024
9 checks passed
@jerelmiller jerelmiller deleted the pr/externalUrlCheck branch January 30, 2024 22:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants