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

[Docs] Add scrollbar to the side navigation menu #1346

Merged
merged 1 commit into from
May 16, 2023

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented May 15, 2023

Description of the changes

Extracted from #1241.

How to test this PR?

I created a RTD job to see the rendering: https://gramine.readthedocs.io/en/dimakuv-docs-add-sidebar-gramine-css/. Notice the scrollbar. Compare with our classic: https://gramine.readthedocs.io/


This change is Reviewable

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


Documentation/_static/css/gramine.css line 8 at r1 (raw file):

.wy-side-scroll {
    width: auto;
    overflow-y: auto;

why not overflow-y: scroll?

Code quote:

auto

Co-authored-by: Deb Taylor <deb.taylor@intel.com>
Co-authored-by: Paul Cartee <paulx.alan.cartee@intel.com>
Co-authored-by: Benny Fuhry <benny.fuhry@intel.com>
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Signed-off-by: Deb Taylor <deb.taylor@intel.com>
Signed-off-by: Paul Cartee <paulx.alan.cartee@intel.com>
Signed-off-by: Benny Fuhry <benny.fuhry@intel.com>
Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


-- commits line 2 at r1:
[Docs] Add scrollbar to the side navigation menu when content overflows

Code quote:

[Docs] Add scrollbar to the side navigation menu

Documentation/_static/css/gramine.css line 8 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

why not overflow-y: scroll?

UPDATE: OK I see, with auto, the scroll bar'll be displayed only if the content is overflowing (i.e., if content fits inside the element's padding box, it looks the same as with visible).

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)


Documentation/_static/css/gramine.css line 8 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

why not overflow-y: scroll?

But why? auto seems like a meaningful default (only show the scrollbar when the table of contents really overflows).

For details, one can check https://www.w3schools.com/cssref/css3_pr_overflow-y.php

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


Documentation/_static/css/gramine.css line 8 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

But why? auto seems like a meaningful default (only show the scrollbar when the table of contents really overflows).

For details, one can check https://www.w3schools.com/cssref/css3_pr_overflow-y.php

Yes, auto makes better sense.

@dimakuv dimakuv force-pushed the dimakuv/docs-add-sidebar-gramine-css branch from ccb6572 to 0dfa466 Compare May 15, 2023 09:48
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)


-- commits line 2 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

[Docs] Add scrollbar to the side navigation menu when content overflows

Done (removed the to make the title char size smaller)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

@dimakuv
Copy link
Author

dimakuv commented May 15, 2023

Jenkins, retest check-python-platlib please (internet connectivity problems)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow mkow merged commit 0dfa466 into master May 16, 2023
@mkow mkow deleted the dimakuv/docs-add-sidebar-gramine-css branch May 16, 2023 00:03
Sign up for free to 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