Skip to content

Conversation

@OmkarSonawane1230
Copy link
Contributor

Key changes include:

  1. Improved Side Navbar: Remove the bullets, doesn't shrink when open some tabs and make the selected tab item bold.
  2. Elimination of Horizontal Scrollbars: Horizontal scrollbars are now hidden in specific tabs
  3. Tab Indicator: Implemented a visual indicator in the navbar to clearly identify the currently opened tab.

Before

Screenshot 2024-05-18 at 7 34 36 PM

After

Screenshot 2024-05-18 at 7 34 36 PM

**In smaller screen size**

BeforeAfter
Before ImageAfter Image

 modified: css/main.css new file: script/script.js
@chriscool
Copy link
Collaborator

Except my question about a code comment, it LGTM. I'd still like @sivaraam to take a look though. Thanks!

@sivaraam
Copy link
Member

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.

})

// if no other tab is active, then the Home tab becomes active
if (!flag) NAVBAR_LI_ELEMENTS[0].classList.add('active'); No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be a good idea since this would activate the "Home" tab for archive links like the one below:

https://git.github.io/rev_news/2024/01/31/edition-107/

I suppose it would be better to just not mark any tab as active for such cases.

@OmkarSonawane1230
Copy link
ContributorAuthor

@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.

@sivaraam
Copy link
Member

@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.

@OmkarSonawane1230
Copy link
ContributorAuthor

@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.

@sivaraam
Copy link
Member

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:

extra-space-side-git-dev-page.mp4

@OmkarSonawane1230
Copy link
ContributorAuthor

@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.

@sivaraam
Copy link
Member

Looks great. Thanks, @OmkarSonawane1230 !

@sivaraamsivaraam merged commit c314067 into git:masterJun 29, 2024
@chriscool
Copy link
Collaborator

Thanks @OmkarSonawane1230 and @sivaraam !

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@OmkarSonawane1230@chriscool@sivaraam