Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
BrickHack 8 Site - Contact & Footer #1256
Changes from 2 commits
197aed6
2f54cb3
5adff61
d363f34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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
(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.
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