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

Allow customizing of blog author image URL documentation #577

Merged
merged 6 commits into from
Apr 18, 2018

Conversation

amyrlam
Copy link
Contributor

@amyrlam amyrlam commented Apr 17, 2018

Motivation

@yangshun and I discussed this issue at our Week 2 meeting. Let me know what you think!

Have you read the Contributing Guidelines on pull requests?

Yes!

Test Plan

First I verified that this functionality already existed by changing Joel's image to Daria.

image

//

Then I updated the docs. I thought it made sense to rename authorImage to authorImageURL, which matches existing authorURL and is more self-documenting.

IS:
image

WAS:
image

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 17, 2018
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.

Looks good except that we should document the authorImageURL field instead as a separate field and make it clear that users should only specify one of them. If they specify both, authorImageURL will override authorFBID.

Out of the top Docusaurus users, it seems like only react-native-website's blog is using authorImage. We will have break the blog if we merge this with the name change. Then again, since it's undocumented, it's probably acceptable to break it.

I think the best way moving forward is to:

  1. Add in authorImageURL and document it
  2. Add support for both authorImage and authorImageURL in Docusaurus
  3. Send a PR to react-native-website to migrate them from authorImage to authorImageURL
  4. Remove support for authorImage in Docusaurus

cc @JoelMarcey @hramos for opinions

@@ -32,7 +32,7 @@ authorFBID: 503283835
title: Introducing Docusaurus
---

Lorem Ipusm..
Lorem Ipusm...
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're touching this line, could you correct it to Ipsum? 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -42,7 +42,7 @@ The only required field is `title`; however, we provide options to add author in

- `author` - The text label of the author byline.
- `authorURL` - The url associated with the author. This could be a Twitter, GitHub, Facebook account, etc.
- `authorFBID` - The Facebook ID that is used to extract the profile picture.
- `authorFBID` - The Facebook ID that is used to extract the profile picture. (Note: If you prefer to use a different image than the Facebook profile picture, put `authorImageURL` instead.)
Copy link
Contributor

Choose a reason for hiding this comment

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

authorImageURL should be documented as a separate field

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 updated the documentation. Let me know if you want any edits, was tricky to know the best way to write this one clearly.

@@ -56,11 +56,11 @@ class BlogPost extends React.Component {
</a>
</div>
);
} else if (post.authorImage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize there already exists an option to customize the author thumbnail via authorImage because it's undocumented...

@JoelMarcey
Copy link
Contributor

@yangshun I agree with your approach to slowly phase out the old name. 👍

We will remove support for authorImage in favor of authorImageURL, after we remove authorImage from react-native-website
@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 18, 2018

I posted updates for items 1 and 2 in this branch - let me know what you think.

I created a branch for 3 here where I replaced authorImage with authorImageURL. (I didn't want to PR it yet since this PR is still open.)

@yangshun yangshun merged commit 57cddb4 into facebook:master Apr 18, 2018
@yangshun
Copy link
Contributor

Looks great! 🎉

@yangshun
Copy link
Contributor

@JoelMarcey Could you release a new version of Docusaurus so that we can update sites that depends on authorImage and move them to authorImageURL?

@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 18, 2018

Thanks! For 3, posted this: facebook/react-native-website#314

@JoelMarcey
Copy link
Contributor

@yangshun I think I can get out a version today.

@JoelMarcey
Copy link
Contributor

@yangshun #581

amyrlam added a commit to amyrlam/Docusaurus that referenced this pull request May 8, 2018
authorImage was replaced by authorImageURL in facebook#577

authorImage was removed in react-native-website in facebook/react-native-website#314
amyrlam added a commit to amyrlam/Docusaurus that referenced this pull request May 8, 2018
authorImage was replaced by authorImageURL in facebook#577

authorImage was removed from react-native-website in facebook/react-native-website#314 

authorImage was undocumented, so no docs changes
@amyrlam amyrlam mentioned this pull request May 8, 2018
amyrlam added a commit to amyrlam/Docusaurus that referenced this pull request May 8, 2018
authorImage was replaced by authorImageURL in facebook#577

authorImage was removed from react-native-website in facebook/react-native-website#314 

authorImage was undocumented, so no docs changes
yangshun pushed a commit that referenced this pull request May 8, 2018
authorImage was replaced by authorImageURL in #577

authorImage was removed from react-native-website in facebook/react-native-website#314 

authorImage was undocumented, so no docs changes
@amyrlam amyrlam deleted the amy/authorImage branch May 11, 2018 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants