-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Enhance footer with Facebook and Twitter buttons #666
Conversation
Deploy preview for docusaurus-preview ready! Built with commit ac315a9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! There are a couple of minor issues which I have commented on. Could you have a look at them first?
docs/api-site-config.md
Outdated
@@ -154,6 +154,8 @@ h1 { | |||
|
|||
`twitter` - Set this to `true` if you want a Twitter social button to appear at the bottom of your blog posts. | |||
|
|||
`twitterId` - If you want a Twitter follow button in the footer, provide a twitter id to follow. For example: `docusaurus`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this to twitterUsername
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for whether to load the Twitter buttons script in Site.js
currently only looks at config.twitter
. It should be changed to config.twitter || config.twitterUsername
because some people might want the follow button but not the blog post Tweet button.
website/core/Footer.js
Outdated
{props.config.projectName} | ||
</a> | ||
</div> | ||
{props.config.twitterId ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use &&
for conditional rendering -
{props.config.twitterUsername && <div> ... </div>}
@yangshun Thank you for the blazingly fast reply & suggestions. Changes added Test plan:
siteConfig docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks!
I had one comment.
website/core/Footer.js
Outdated
const SocialFooter = props => ( | ||
<div> | ||
<div> | ||
<h5>GitHub</h5> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need the headings. The social buttons are pretty self-describing in my opinion. I think it looks cleaner without them.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoelMarcey
I am not really sure. I'll let you decide 😄
Here are some A-B testing (more like A-B-C) for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like one of the first two better 😄
But, I am going to let a real UI person decide - what do you think @yangshun ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second one looks best to me! I do agree that the titles are kinda redundant, so the third is my least favorite. How do they look like on mobile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me. #2, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR commit updated using #2.
Extra thing to note is that the share dialog button won't work because the facebook app not being set to public.
App Not Setup: This app is still in development mode, and you don't have access to it.
Issue related is #625 . But after that issue is fixed, this will work as well.
I think we also need to update the doc on facebookAppId
so that user will make sure to set their app to public (not development mode)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
@yangshun Our app id in siteConfig.js
isn't working, you say?
I will let you stamp this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! This looks great 😄
Motivation
This feature/enhancement was requested in #218 .
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Test 4 cases by tweaking siteConfig.js:
4.. No twitter (facebookAppId exist & twitterId does not exist)
Check updated docs for api siteconfig
docs
from .npmignoreRun test in root folder
Related Issues
#625 Facebook share dialog won't work if app is not set to public