Closed OmkarSonawane1230 closed 2 months ago
Except my question about a code comment, it LGTM. I'd still like @sivaraam to take a look though. Thanks!
This works better than the previous version. That said, I think we could still achieve stickiness in the sidebar without setting a max-height
for the whole page. Checkout the changes in this branch to get an idea of the kind of stickiness I mean. I suggest this as I believe this would ensure the page would have an appropriate height with respect to the content rather than blindly setting it as 100% of the view port.
Further, once the changes have been finalized, it would be ideal if we cross-check with @mjaix to ensure these changes don't affect the e-mail version of the newsletter he sends out each month.
@sivaraam if we don't set the main wrapper
height to 100% then we can't apply the sticky property to the navbar
because it is inside the wrapper
, which make it move upward as page is scrolled, because the scroll is apply to the html
it self. if we set the wrapper
height 100%, then we can apply position sticky to navbar
which will work as expected.
@OmkarSonawane1230 I actually gave it a shot already. Try checking out the code in the following branch: https://github.com/git/git.github.io/tree/sticky-without-max-height
You will see the sticky achieved without setting the max-height
for the whole wrapper.
@sivaraam Thank you for your feedback. I have addressed the issues you mentioned in the code review.
I followed your suggestion to refactor the max-height
and sticky position
problem. I was using safari so the position: sticky
was not working on my side. but now it is working by adding position: -webkit-sticky
.
Thanks again for your guidance on this.
The changes now look good for the most part. There's just this one issue I noticed which doesn't exist in the current webpage. On smaller screens there's some additional space on the right which is resulting in an unnecessary horizontal scrollbar even though there isn't any content displayed there. Could you check the cause of this?
Here's a screen recording about the issue:
https://github.com/git/git.github.io/assets/12448084/95c39bd2-9893-41e1-8a3c-99be830f1d0c
@sivaraam the issue was caused navbar.
The navbar
was causing the horizontal scrollbar because of width: 100%
, so I changed it to auto
now it works fine and the horizontal scroll is not visible any more.
Looks great. Thanks, @OmkarSonawane1230 !
Thanks @OmkarSonawane1230 and @sivaraam !
Key changes include:
Before
After
**In smaller screen size**