-
Notifications
You must be signed in to change notification settings - Fork 30
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
BrickHack 8 Site - Initial Release #1251
Conversation
✔️ Deploy Preview for infallible-lumiere-e9d357 ready! 🔨 Explore the source changes: 7d455d3 🔍 Inspect the deploy log: https://app.netlify.com/sites/infallible-lumiere-e9d357/deploys/61d146aef64dad00073af4e2 😎 Browse the preview: https://deploy-preview-1251--infallible-lumiere-e9d357.netlify.app/ |
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 is AWESOME! Fantastic refactor; they're never easy and you manged to cleanup the existing styles while also preserving the functionality we needed!! 🥳
As I mentioned at some meeting or another, I've not really reviewed the existing sections too much; I'll wait for the more-final PRs for each section to really give that a fine-tooth comb; it looks like all of them need at least some tweaking.
Biggest comment is the font loading issue; I don't think Work Sans
is provided by default on macOS; it either needs to be loaded thru Google Fonts or statically (by us) in assets/
, hence why comment
vs. approve
.
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.
LGTM
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.
Looks great!
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.
Really really great job on this. Can't wait to show it off!
Checked through all links and clickable stuff, everything seems good.
Questions:
- MLH banner looks a little "lopsided" cause it doesn't have the same leading margin as the rest of the site content (particularily navbar).
- (Also we aren't MLH confirmed iirc; I think we have to hide the banner for release, no?)
Suggestions(tm)
- If you+design would like, mobile can have a little less margin on left/right cause of space constraints, but that's to taste. (Especially expanded FAQ sections; iPhone 12 has a thick "double margin")
- Centered text/login/register block on mobile? (Text stays left aligned but content center?)
Commented out for now. We can fix any lopsidedness when it goes in.
Design had no comment on this in their overall review, but if it is looking too big to you on your phone can you send a screenshot to #design and ask them?
Again, that would be something to screenshot and ask design channel about specifically. They might want to keep alignment consistent but idk if it looks funky on your screen size specifically |
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.
👍 LGTM!
Co-authored-by: Chris Baudouin, Jr. <[email protected]>
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.
(not yet cause HE verify stuff but) WOOOO LGTM!
Fixes #1246
Status: Review time :)