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 name underneath users logo #1043

Closed
wants to merge 5 commits into from

Conversation

mptap
Copy link

@mptap mptap commented Oct 15, 2018

Motivation

Added name of user underneath logo on users page #1040

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Ran yarn prettier, yarn lint and yarn test

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 15, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 275fdfc

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

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

@mptap Thank you for the PR. 👍

I think this may require some CSS updates, possibly, given what it looks like in the preview. https://deploy-preview-1043--docusaurus-preview.netlify.com/en/users

@mptap
Copy link
Author

mptap commented Oct 15, 2018

@JoelMarcey Updated, let me know what you think.

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

I think the name should be underneath the logo. Seems more natural to me.

@mptap
Copy link
Author

mptap commented Oct 16, 2018

@JoelMarcey Updated

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.

Non-square images will be distorted if you change the CSS in that manner. Please have a look again.

@@ -2220,14 +2220,14 @@ input::placeholder {
}

.showcaseSection .logos img {
max-height: 128px;
height: 128px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break logos that aren't square in proportion. For example, have a look at Vuls and ml5.js on https://deploy-preview-1043--docusaurus-preview.netlify.com/en/users and compared them with https://docusaurus.io/en/users. The images are distorted.

You could add a wrapper div that has fixed width and height around all the images so that you can align the names and still maintain the aspect ratio of the images.

Copy link
Author

Choose a reason for hiding this comment

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

@yangshun Updated, waited for the deploy env to be built - but it is on hold for some reason

padding: 20px;
width: 128px;
}

@media only screen and (max-width: 735px) {
.showcaseSection .logos img {
max-height: 64px;
height: 64px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Author

Choose a reason for hiding this comment

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

@yangshun Updated, waited for the deploy env to be built - but it is on hold for some reason

@yangshun yangshun changed the title Add name of user underneath logo on users page #1040 feat: add name underneath users logo Oct 18, 2018
@endiliey endiliey changed the title feat: add name underneath users logo chore: add name underneath users logo Oct 20, 2018
@endiliey endiliey changed the title chore: add name underneath users logo feat: add name underneath users logo Oct 20, 2018
@mptap mptap force-pushed the add_user_name branch 2 times, most recently from 3e5ae00 to 6f23219 Compare October 25, 2018 04:51
<a href={user.infoLink} key={user.infoLink}>
<img src={user.image} alt={user.caption} title={user.caption} />
</a>
<div>
Copy link

Choose a reason for hiding this comment

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

The key-prop needs to be moved to this div

@yangshun
Copy link
Contributor

yangshun commented Nov 2, 2018

Apologies @mptap we decided to go with another implementation. Thanks for your efforts 😄

@yangshun yangshun closed this Nov 2, 2018
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.

6 participants