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(v1): self-host user images #3200

Merged
merged 14 commits into from
Aug 8, 2020
Merged

fix(v1): self-host user images #3200

merged 14 commits into from
Aug 8, 2020

Conversation

leoigel
Copy link
Contributor

@leoigel leoigel commented Aug 3, 2020

Motivation

(Write your motivation here.)
Improve reliability

Have you read the Contributing Guidelines on pull requests?

Yes
(Write your answer here.)
In https://docusaurus.io/en/users i saw one image not loaded.

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!)
I replaced all external url to self-host all images.

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

@leoigel leoigel requested a review from yangshun as a code owner August 3, 2020 18:23
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 3, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 3, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 4bf198d

https://deploy-preview-3200--docusaurus-2.netlify.app

@leoigel
Copy link
Contributor Author

leoigel commented Aug 3, 2020

Some thing is missing here? the pr was approved?

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks, That almost looks good.

Can you please ensure all logos have clearly identifiable names? (rename the original filename if needed)

Also there's one broken path

If you could also add a check to ensure we don't add non-self-hosted logos anymore, that would be cool

For example:

users.forEach(user => {
  if ( user.image.startsWith("/img/users") ) {
    throw new Error("User image should be self-hosted in /img/users folder. This was not the case for " + user.image)
  }
});

infoLink: 'https://pnpm.js.org/',
fbOpenSource: false,
pinned: false,
},
{
caption: 'Polymath Network',
image: 'https://developers.polymath.network/img/text.svg',
image: '/img/users/polymatch-logo.png',
Copy link
Collaborator

Choose a reason for hiding this comment

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

how comes it's not a png?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took png image from official website https://polymath.network/

@@ -737,7 +737,7 @@ module.exports = [
},
{
caption: 'smash.gg',
image: 'https://imgur.com/eBFBDei.png',
image: '/img/users/eBFBDei.png',
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename it please

@@ -317,7 +317,7 @@ module.exports = [
},
{
caption: 'Home Assistant',
image: 'https://developers.home-assistant.io/img/logo-responsive.svg',
image: '/img/logo-responsive.svg',
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename it + fix the link (missing /users in path)

@leoigel leoigel requested a review from slorber August 7, 2020 15:50
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM except the image path check that shouldn't be in a comp

</div>
);
const Showcase = ({users}) => {
users.forEach((user) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this check should be outside of the render method, at the top level

@slorber slorber linked an issue Aug 7, 2020 that may be closed by this pull request
@slorber slorber mentioned this pull request Aug 7, 2020
@slorber slorber changed the title img-self-hosted fix(v1): self-host user images Aug 7, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 7, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 8, 2020

thanks!

@slorber slorber merged commit fd403d3 into facebook:master Aug 8, 2020
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.

Image not loaded
4 participants