-
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 - Contact & Footer #1256
Conversation
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 good! I left a few nits, but nothing very important.
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.
literally the tiniest of nits, but all of the other post-review changes look good 👍
Co-authored-by: Skye Gallup <[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.
Great work! Just one question
sass/index.scss
Outdated
margin: 2rem auto -2px; | ||
max-width: 75%; | ||
width: 75%; | ||
height: calc((100vw - 10rem) * 0.56); // should be the same as 75% width |
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.
Why the calc vs. 75vw? (And hwere does 0.56 come from?)
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.
In the image's aspect ratio and original desktop styling, the height is ~75% of the width. Lines 367 - 369:
width: 430px;
max-width: 50%;
height: 330px;
I want to maintain this aspect ratio when the width is set to 75% as it is here. There's a few steps that it takes to get to the right number
75% width is not actually 75vw here. It's 75% of the space that the container is allowed to fill. So 100vw minus the padding for the section, which is a total of 7rem. (I mistakenly put 10 in the code here because the desktop section padding adds up to 10rem in width. I will be changing that after I post this comment.) So, to get the 100% value we do 100vw - 7rem
Then, we need to multiply by 75% to get the width of the image. Then by 75% again to get the height to be 75% of the width value. 0.75 * 0.75 is ~0.56
So in the end, getting roughly the same aspect ratio leaves us with a height of (100vw - 7rem) * 0.56
If you double check my work, keep in mind that the box model breakdown in inspect tool shows width of container and padding as seperate. We have the css set to include the padding as a part of the total width value. As I remembered while trying to go back and check my work to explain it.
Actually changing this to 75vw severely messes with it.
When I go to fix the 10rem to 7rem, I will try to clarify my comment a bit more.
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.
It's 75% of the space that the container is allowed to fill.
Ahhhhh I see, I forgot how percents worked lol
In that case I'd suggest making a variable or two. This can be done in general PR as you clarified all the logic here so don't consider this to be a blocker: but maybe
height: calc(((100vw - var(--section-pad)) * (0.75 * 0.75))) // squared to match height and width
(I don't fully understand the 0.75 * 0.75
but I shall take your word for it)
Or you can just link to this comment; I do that a lot when there's a lot of nuances depending on the current state of the code
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.
I don't fully understand the 0.75 * 0.75
Width of the image is 0.75 * the total possible width (width: 75%)
Height of the image needs to be 75% of the width, aka 0.75 * width aka 0.75 * 0.75 * the total possible width
If that makes sense
The comment in your sample is inaccurate since it's not "to match"
I could add a variable for section padding, but since section padding is probably going to be changed in general pr anyways I agree to save it for then
LGTM |
👍 still LGTM |
* swapping files and start gutting bh7 copy * more gutting of css * style refactor * logo update * design fixes * mobile navbar * design fixes * social cards being funky * editing social card resolution * adjusting social card * favicon swap * nav underline * outline color fix * remove login * fixes * comment fixes * contact first attempt * changing social links * Revert "comment fixes" This reverts commit 75f57c6. * Revert "Revert "comment fixes"" This reverts commit 6c75a58. * Revert "changing social links" This reverts commit fef9a7b. * Revert "contact first attempt" This reverts commit d980e9a. * BrickHack 8 Site - Hero Section (#1253) * starting hero * hero section * swapping out loop file * fixes * design fixes * spacing fix * BrickHack 8 Site - About Section (#1254) * inital about section * card width * fixes * BrickHack 8 Site - FAQ Section (#1252) * faq styling + ricky * fixing mobile * removing old padding class * fixes * fixing broken cards * BrickHack 8 Site - Contact & Footer (#1256) * contact section * fixes * newline eof Co-authored-by: Skye Gallup <[email protected]> * fixing aspect ratio code Co-authored-by: Skye Gallup <[email protected]> * design + js fixes * design fixes * trying to hide play button * trying to hide play button 2 * trying to hide play button 3 * fixing faq ricky * fixing mobile grayout opacity * hiding mlh banner * copyright date * updating stats * prepping for open applications * content fixes for chris * spelling error Co-authored-by: Chris Baudouin, Jr. <[email protected]> * fixing spelling error * fixing faq padding * hero content adjustments * hero content addition * centering hero on mobile Co-authored-by: Skye Gallup <[email protected]> Co-authored-by: Chris Baudouin, Jr. <[email protected]>
Fixes #1249 --finally
Need to ask design about some of the stuff I did for social media link styling and mobile styling