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

improve team display #839

Merged
merged 2 commits into from
Apr 26, 2023
Merged

improve team display #839

merged 2 commits into from
Apr 26, 2023

Conversation

JustinStitt
Copy link
Collaborator

I was browsing the site and realized how poor the team display is on the home page.

This PR aims to make it more bearable (also tall display support!)

OLD

team-display1
notice how the text doesn't line up and the logos cover the text

NEW (with tall display support)

team-display2
fixed height logos, tall display utilizes stacked display

Various resizing stuff

team-display3

This class was for dev purposes, no longer needed. Shouldn't have been
in PR to begin with.
@netlify
Copy link

netlify bot commented Apr 26, 2023

Deploy Preview for acmcsuf ready!

Name Link
🔨 Latest commit 4fddfdd
🔍 Latest deploy log https://app.netlify.com/sites/acmcsuf/deploys/6448c572af7493000884a3b1
😎 Deploy Preview https://deploy-preview-839--acmcsuf.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@karnikaavelumani karnikaavelumani left a comment

Choose a reason for hiding this comment

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

Thank you so much Justin for your PR! I believe I made an issue for this somewhere so if I remember I will reference and close that issue.

Can you make the badges just a bit smaller before you merge? Overall it looks amazing!

@JustinStitt
Copy link
Collaborator Author

Thank you so much Justin for your PR! I believe I made an issue for this somewhere so if I remember I will reference and close that issue.

Can you make the badges just a bit smaller before you merge? Overall it looks amazing!

What badges? The team logos? I didn't change their size on Desktop version. If you mean on the tall display version then yeah, let me know by what % they should shrink by.

The idea for the tall display stacked version is that the logo is approximately the same size as the text to sell the effect a bit better.

Please pull my branch locally and experiment with a better size for the logos.

@karnikaavelumani
Copy link
Collaborator

I apologize I meant the logos. The reason for the overflow was because I updated the assets with no padding. Before when there was padding, the logos looked a bit smaller.

You might have to change the width now or just add padding to the img tag.

@JustinStitt
Copy link
Collaborator Author

I apologize I meant the logos. The reason for the overflow was because I updated the assets with no padding. Before when there was padding, the logos looked a bit smaller.

You might have to change the width now or just add padding to the img tag.

Honestly, out of scope of my PR. Create an issue (or revive an old issue) and create a new PR after this one is merged.

It's meaningless to discuss how many pixels the logos should be smaller by then I go and do it and update my PR only to be off by a few pixels of what is desired.

I was mainly concerned with text alignment and tall display styles.

@karnikaavelumani
Copy link
Collaborator

Sounds good! That's why I approved this PR in the first place. I will take that advice and apply it.

Feel free to merge!

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution, @JustinStitt. I’ll go ahead and merge this PR in a moment.

@EthanThatOneKid EthanThatOneKid merged commit 88778ab into main Apr 26, 2023
Copy link
Contributor

@jaasonw jaasonw left a comment

Choose a reason for hiding this comment

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

nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants