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

fix(v2): fix external URL for og:image tag #2420

Merged
merged 7 commits into from
Mar 18, 2020
Merged

fix(v2): fix external URL for og:image tag #2420

merged 7 commits into from
Mar 18, 2020

Conversation

38elements
Copy link
Contributor

@38elements 38elements commented Mar 16, 2020

Motivation

// docusaurus.config.js
module.exports = {
  ...
  themeConfig: {
    // Relative to your site's "static" directory.
    // Cannot be SVGs. Can be external URLs too.
    image: 'img/docusaurus.png',
    ...
  },
}

https://github.com/facebook/docusaurus/blob/2b6e4409704fb20a39a9273b1b051065f0eb8079/website/docs/theme-classic.md#meta-image

Currently, when url is setted to themeConfig.image, this does not work as below.

https://example.comhttps://example.com/img/foo.png

This fixes it.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 16, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Mar 16, 2020

Deploy preview for docusaurus-2 failed.

Built with commit 6d5c797

https://app.netlify.com/sites/docusaurus-2/deploys/5e71f246330f8100084ac4e6

@lex111 lex111 changed the title fix(v2): og:image fix(v2): fix external URL for og:image tag Mar 17, 2020
@lex111 lex111 added this to the v2.0.0-alpha.49 milestone Mar 17, 2020
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Nice!

@38elements
Copy link
Contributor Author

38elements commented Mar 17, 2020

@lex111

This does not work normally.
I will add useMetaImageUrl().

export default function useBaseUrl(url) {
const {siteConfig} = useDocusaurusContext();
const {baseUrl = '/'} = siteConfig || {};
if (!url) {
return url;
}
const externalRegex = /^(https?:|\/\/)/;
if (externalRegex.test(url)) {
return url;
}
if (url.startsWith('/')) {
return baseUrl + url.slice(1);
}
return baseUrl + url;
}

export default function isInternalUrl(url) {
return /^\/(?!\/)/.test(url);
}

/^\/(?!\/)/.test('/foo.jpg') // true
/^\/(?!\/)/.test('foo.jpg') // false
/^\/(?!\/)/.test('foo/foo.jpg') // false

@lex111
Copy link
Contributor

lex111 commented Mar 17, 2020

@38elements thank you for the report. We will rework this function, so do not change this PR.

@38elements
Copy link
Contributor Author

38elements commented Mar 17, 2020

@lex111
Thank you for reply.
I do not change this pull request.
I think it is better to make useMetaImageUrl(siteUrl, metaImage).
Since there is same code as below in 2 file.
I hope someone makes it, after this pull request is merged.

import isInternalUrl from '@docusaurus/isInternalUrl';
// ...
  let metaImageUrl = siteUrl + useBaseUrl(metaImage);
  if (!isInternalUrl(metaImage)) {
    metaImageUrl = metaImage;
  }

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thank you, this is great!

@yangshun
Copy link
Contributor

I think we need some sort of normalizing function that will ensure between each URL segment there's only one /. Sometimes people add / before and/or after the baseUrl and url in docusaurus.config.js and if we just naively append them, the resulting value might not be a valid URL.

@docusaurus/utils has it but it's not used on the client side, only in plugins.

@yangshun yangshun merged commit 8152b47 into facebook:master Mar 18, 2020
@yangshun yangshun added the pr: bug fix This PR fixes a bug in a past release. label Mar 18, 2020
@yangshun
Copy link
Contributor

Oops premature merging. Probably some formatting issue. I'll fix it later tonight, sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants