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

Show the right sidebar on wider screens if the left is hidden #570

Merged
merged 6 commits into from Apr 21, 2022

Conversation

jorisvandenbossche
Copy link
Member

Experiment for #568

@choldgraf
Copy link
Collaborator

I don't see any obvious difference in the right sidebar when the left sidebar is hidden.

Hidden sidebar: https://pydata-sphinx-theme--570.org.readthedocs.build/en/570/demo/no-sidebar.html
Not hidden: https://pydata-sphinx-theme--570.org.readthedocs.build/en/570/demo/index.html

@jorisvandenbossche are you still interested in working on this PR?

@damianavila
Copy link
Collaborator

@choldgraf you need to compare the left hidden sidebar example in this PR:

against the behavior in stable:

If you narrow the horizontal space, you will see a different behavior.

@choldgraf choldgraf changed the title Give more space to right sidebar if left one is hidden Show the right sidebar on wider screens if the left is hidden Feb 24, 2022
@choldgraf
Copy link
Collaborator

Ah I see - I've re-titled the PR so that it is more reflective of the current change. I was looking for a change in width of some kind, but I see it is only the change in the screen width at which the right sidebar is hidden.

I'm fine with this change, though I'm worried that the implementation is going to be confusing for people - all of these conditional bootstrap class checks are a bit hard to wrap your head around without the context of what they're doing. So I guess I'm +0, but we definitely should add comments to the changes here so that a person with no context would understand what they're doing

@damianavila
Copy link
Collaborator

So I guess I'm +0, but we definitely should add comments to the changes here so that a person with no context would understand what they're doing

I agree with your thoughts here, @choldgraf

@damianavila
Copy link
Collaborator

but we definitely should add comments to the changes here so that a person with no context would understand what they're doing

Any feeling about the depth of the comments you want to add here @choldgraf?

@damianavila
Copy link
Collaborator

Any feeling about the depth of the comments you want to add here @choldgraf?

Pinging @choldgraf again on this one 😉.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I added a few quick commits to add more comments, and to use one if statement that sets variables and re-uses them, rather than two. I think that this makes it easier to read and understand. I'm +1 on merging this one and seeing how people like it.

@choldgraf choldgraf merged commit 2e88b0f into pydata:master Apr 21, 2022
@choldgraf
Copy link
Collaborator

Merging this in since we don't have any suggestions otherwise! We can tweak this over time

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.

None yet

3 participants