Skip to content
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

Allocate space for scrollbar in sitenav and pagenav #2491

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

jingting1412
Copy link
Contributor

@jingting1412 jingting1412 commented Apr 2, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
#2217
Changed the property value of overflow-y of the .nav-component to scroll to allocate space to the scrollbar as discussed here so that its appearance and disappearance doesn't cause the icon in the nav bar to shift.

Since this change is to .nav-component, both the sitenav and pagenav is affected.

Since the change is to the main.css file of the default template, all the other main.css files also have to be updated. Currently there isn't a way to automatically update the file after a MarkBind upgrade (#1961), so if this change is accepted, there has to be release note for current users to update their main.css file.

Anything you'd like to highlight/discuss:
overflow-y: overlay is a legacy value alias for overflow-y: auto according to the mdn docs here, which unfortunately still causes the moving of the content from what I can see.

The overlay value of the overflow CSS property overall is being deprecated and standardised as the scrollbar-gutter property, but I see that its not really supported on Safari browser. If anyone wants me to look into using this property instead, or any other alternatives, I'll be happy to try it out.

Previous behaviour:

Screen.Recording.2024-04-03.at.12.56.40.AM.mov

Current behaviour:

Screen.Recording.2024-04-03.at.12.57.56.AM.mov

Testing instructions:
Serve the docs using markbind serve -d, collapse all sections of the siteNav, open the sections one by one until the scrollbar appears. Ensure that when the scrollbar appears, the icons doesnt shift.

Proposed commit message: (wrap lines at 72 characters)
Allocate space for scrollbar in nav components


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.14%. Comparing base (69ec838) to head (625134d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2491   +/-   ##
=======================================
  Coverage   51.14%   51.14%           
=======================================
  Files         124      124           
  Lines        5359     5359           
  Branches     1152     1152           
=======================================
  Hits         2741     2741           
  Misses       2328     2328           
  Partials      290      290           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LamJiuFong
Copy link
Contributor

Thanks for the work @jingting1412!

I took a look at scrollbar-gutter and if I'm not wrong scrollbar-gutter does the same thing as overflow-y: scroll right?

I am good with having a blank space here (next to the sitenav) while hovering
Screenshot 2024-04-03 at 1 33 41 AM

not sure how others think

@yucheng11122017
Copy link
Contributor

Hmm does this mean that the scrollbar will be present even if the site-nav components' height is small and doesn't need a scrollbar?

@jingting1412
Copy link
Contributor Author

Hmm does this mean that the scrollbar will be present even if the site-nav components' height is small and doesn't need a scrollbar?

The space allocated for the scrollbar will always be there yes but the scrollbar will only appear when it overflows. The behaviour is the same as

overflow: overlay
scrollbar-gutter: stable

@yucheng11122017
Copy link
Contributor

Hmm does this mean that the scrollbar will be present even if the site-nav components' height is small and doesn't need a scrollbar?

The space allocated for the scrollbar will always be there yes but the scrollbar will only appear when it overflows. The behaviour is the same as

overflow: overlay
scrollbar-gutter: stable

I see. Thanks for the explanation!

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM except for that diagram thing! Would need to fix the other issue first but I think this can be merged in first

Copy link
Contributor

Choose a reason for hiding this comment

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

Ongoing issue due to #2498

@yucheng11122017 yucheng11122017 merged commit e2f6301 into MarkBind:master Apr 5, 2024
10 checks passed
@github-actions github-actions bot added the r.Patch Version resolver: increment by 0.0.1 label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Patch Version resolver: increment by 0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants