-
Notifications
You must be signed in to change notification settings - Fork 1
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
Updated Sponsor Styling #154
base: main
Are you sure you want to change the base?
Conversation
Deploy preview for zothacks-site-2023-sanity ready!
|
Deploy preview for zothacks-site-2023 ready!
|
@include utils.media-breakpoint-down(sm) { | ||
font-size: 2rem; | ||
} |
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.
Suggestion [repeating]: Bootstrap recommends a "mobile-first" design approach, so making the narrower view the default and using media-breakpoint-up
would be more appropriate.
flex-wrap: wrap; | ||
justify-content: center; | ||
gap: 1.5rem; | ||
max-width: 1200px; |
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.
Suggestion: rather than manually constraining the inner elements, we can use Bootstrap's native container sizing (unfortunately not done last year) by changing the main <section>
element to be a Container
, see Home Intro (Intro.tsx:14
) for example.
width: 100%; | ||
height: 100%; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; |
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.
Suggestion: since the parent elements have .logo
class applied, the centered placement should already be taken care of, so these shouldn't need to be specified.
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.
Think you also have to remove the forced dimensions (width
and height
) for it to be centered by parent
…implementation with project's mobile-first design approach
// .container { | ||
// padding-top: 6rem; | ||
// padding-bottom: 6rem; | ||
// } |
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.
Chore: little value in keeping around old commented code, better to just remove it
width: 100%; | ||
height: 100%; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; |
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.
Think you also have to remove the forced dimensions (width
and height
) for it to be centered by parent
@@ -3,6 +3,7 @@ import { getSponsors } from "./getSponsors"; | |||
import styles from "./Sponsors.module.scss"; | |||
import { client } from "@/lib/sanity/client"; | |||
import imageUrlBuilder from "@sanity/image-url"; | |||
import { Container } from "react-bootstrap"; |
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.
Chore: I forget if Next.js handles this well, but to be safe, we'll want to import just the individual component, i.e. import Container from "react-bootstrap/Container"
Import individual components rather than the entire library. Doing so pulls in only the specific components that you use (React Bootstrap, Importing Components)
Description
Used existing CMS functions for new sponsor section. Updated styling to make it responsive. Added hover effect for fun!!!!
Screenshots
Steps to verify/test this change:
Final Checks:
Issues
Closes #146
Todo
Nice to have
Maybe add more animations??