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

feat: add caption to user links #1075

Merged
merged 4 commits into from
Nov 2, 2018
Merged

feat: add caption to user links #1075

merged 4 commits into from
Nov 2, 2018

Conversation

goksu
Copy link
Contributor

@goksu goksu commented Oct 27, 2018

Created a shared component Showcase and updated the usages around the site. Effects index and users.

Motivation

Fixes #1040.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Clean linter, and visited pages affected.

Related PRs

There was already an open PR #1043 by @mptap, but I've taken a significantly different approach and trying to remove some repetition of code.

created a shared component `Showcase`
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 27, 2018
@goksu goksu changed the title feat: add caption to users links #1040 feat: add caption to users links Oct 27, 2018
@goksu goksu changed the title feat: add caption to users links feat: add caption to Docusaurus user links Oct 27, 2018
@goksu goksu changed the title feat: add caption to Docusaurus user links feat: add caption to user links Oct 27, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 27, 2018

Deploy preview for docusaurus-preview ready!

Built with commit a23407b

https://deploy-preview-1075--docusaurus-preview.netlify.com

endiliey
endiliey previously approved these changes Oct 28, 2018
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Unfortunately, all existing users site using our showcase component will break because we're removing some classes from lib/static/main.css. We cannot rename the classes, and can only add new ones. This is a downside of v1 that will be addressed in v2.

@endiliey endiliey dismissed their stale review October 28, 2018 08:28

See yangshun's review

@goksu
Copy link
Contributor Author

goksu commented Oct 28, 2018

@yangshun — Can you give some details about why that's the case? Is that something related to caching? Also, please let me know if I need to update other parts of the PR that are affected by this other than the main.css.

@yangshun
Copy link
Contributor

@gtoprak I just realized we met at React Conf! Thanks for your contribution!

Hmm it's because lib/static/main.css is part of the docusaurus library. This showcase thing is generated for all projects when someone does docusaurus init and we won't have control over those code after that. If we remove the productShowcaseSection, existing users' HTML will still have the class but there's no longer any styling associated with the class. I recommend not changing any CSS classes. We can only add more classes, but even that is not ideal because ALL users will get the new CSS even though they're using it. Ideally we just add HTML and the custom HTML we add to v1/website/static/css/custom.css.

@goksu
Copy link
Contributor Author

goksu commented Oct 30, 2018

@yangshun — Yes, thank you for the sticker! I am hoping to have many more contributions in the future too.

I've made the updates to custom.css and reverted my changes to main.css. As far as I can tell from the netlify deploy, it seems to be working as it does on my local. But, please do not hesitate to let me know if this work still is not complete, as you have the expertise on how Docusaurus functions for the users, and I'll lean on you on that.

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.

This looks better! However we want the logos to be of the same width and height and the captions are just right underneath. In this current implementation the captions are not aligned at all.

v1/lib/core/CompLibrary.js Outdated Show resolved Hide resolved
v1/lib/core/Showcase.js Outdated Show resolved Hide resolved
v1/lib/core/Showcase.js Outdated Show resolved Hide resolved
v1/lib/static/css/main.css Outdated Show resolved Hide resolved
v1/website/pages/en/users.js Outdated Show resolved Hide resolved
@goksu
Copy link
Contributor Author

goksu commented Oct 31, 2018

I've made the suggested updates, and I did have an off alignment on certain user images. Following two screenshots are how it's looking for me, please let me know if that doesn't reflect what the team is envisioning.

@yangshun — Thank you for the ongoing code review as I appreciate it a lot since I am not familiar with the code base, it's really helpful!

screen shot 2018-10-30 at 5 19 13 pm

screen shot 2018-10-30 at 5 19 34 pm

@endiliey endiliey changed the title feat: add caption to user links chore: add caption to user links Nov 1, 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.

Very awesome. Thank you!

@yangshun yangshun merged commit 3250662 into facebook:master Nov 2, 2018
@yangshun yangshun changed the title chore: add caption to user links feat: add caption to user links Nov 2, 2018
@goksu
Copy link
Contributor Author

goksu commented Nov 2, 2018

Thank you @yangshun!

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.

5 participants