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

Make the sidebar stay on the page during scroll #54

Merged
merged 5 commits into from Nov 30, 2018

Conversation

ReventonC
Copy link
Contributor

@ReventonC ReventonC commented Nov 24, 2018

closes #24

@choldgraf
Copy link
Member

Really nice start! I agree it's much nicer to have it stay in-place.

I gave it a shot, and there seemed to be a few screen sizes where the sidebar would overlap with the text. We need to make sure that things look nice on both narrow (e.g. phones) and wide (e.g. widescreen monitors).

I made an edit that I think resolves this issue. It does so by setting the fixed position from the left, not the right. That way we can set the x-position depending on the width of the content.

Take a look and let me know what you think! Does that work for you?

right: 0;
position: fixed;
top: 8%;
left: 1030px;
Copy link
Member

Choose a reason for hiding this comment

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

This means that the sidebar will always be just to the right of the content, provided the screen is wide enough. (as opposed to your original PR, which would put this at the right of the screen regardless of the size). Is that OK?

@ReventonC
Copy link
Contributor Author

ReventonC commented Nov 26, 2018

Yeah, it looks good for now! However, this is definitely not the best way to solve it. Javascript should also be able to make it float on the page without using position: fixed (maybe using window scroll).
One thing I noticed when we're deploying the jupyter book to our class website is that it doesn't really have a good responsive design. As you said,

there seemed to be a few screen sizes where the sidebar would overlap with the text

We probably should change all the styles in the layout and make it responsive, so that we can solve this problem fundamentally.

@choldgraf
Copy link
Member

Yeah, totally agree. @ReventonC do you have thoughts on whether "make it responsive" should block on this PR getting in for the sake of "good enough right now"? Maybe @SamLau95 has thoughts too

@SamLau95
Copy link
Collaborator

I pushed some changes to this branch that should make the sidebar responsive. Basically, I use flexbox to position the sidebar above the page content. When there's enough horizontal space, I position the sidebar to the right of the page content by switching flex-direction: column to flex-direction: row.

Here's how it looks for me:

sidebar

@choldgraf
Copy link
Member

It looks great for horizontal movement! though demoing it locally I notice two issues:

  1. the sidebar now does not stay in a fixed vertical position when scrolling vertically (that was the original goal of this PR)
  2. the sidebar changes its vertical position depending on whether there is an interact link there. I think we should aim for the sidebar to be in the same vertical location regardless of the content.

@SamLau95
Copy link
Collaborator

SamLau95 commented Nov 28, 2018

You're totally right @choldgraf , I forgot about the fixed part. It took an alarmingly long time to figure out how to make the right sidebar fixed and responsive. This is what I've got now:

sidebar

@choldgraf
Copy link
Member

choldgraf commented Nov 28, 2018

looks great to me! there's something weird with the scrollbar being just to the right of the sidebar, rather than locked to the right side of the page.

image

However, I think this may be platform-dependent, and shouldn't be a blocker on this PR. Better to open another issue about IMO. So unless the fix for the above question is simple, if @SamLau95 and @ReventonC are +1 on this PR, I'm +1 on merge!

@SamLau95
Copy link
Collaborator

I'm +1 for merging this now and fixing the scrollbar in a future PR.

@choldgraf
Copy link
Member

@ReventonC what do you think? It's your PR after all :-)

@ReventonC
Copy link
Contributor Author

Sorry for the late response! And yeah, I think this works!

@ReventonC
Copy link
Contributor Author

I think I will also take a look at the styling over the winter, and see if I can figure some more general solution for the responsive design of jupyter book!

@choldgraf
Copy link
Member

sounds good @ReventonC then we shall 🚢 it!!

Thanks to both of you for the help, this is great improvement :-)

@choldgraf choldgraf merged commit 6b4b076 into executablebooks:master Nov 30, 2018
choldgraf added a commit to choldgraf/jupyter-book that referenced this pull request Apr 28, 2020
updating the configuration parsing and docs around it
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.

Feature request: right-side navbar auto-scroll
3 participants