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 content wider (now back to centered) #183

Merged
merged 8 commits into from
Feb 1, 2024

Conversation

jashapiro
Copy link
Member

@jashapiro jashapiro commented Nov 5, 2023

I was bothered by the version selector not being lined up with the nav bar, and the only way I could see to fix it was to move the nav bar all the way to the left by removing the position: relative from wy-grid-for-nav. But I like it that way anyway?

I also made the body content just a bit larger.

As this was a pretty minor change, I am happy to be overruled, but I thought I would submit for comment at least.

Preview: https://scpca--183.org.readthedocs.build/en/183/

@jashapiro jashapiro changed the base branch from development to documentation-improvements November 6, 2023 16:32
@jashapiro jashapiro changed the base branch from documentation-improvements to development November 6, 2023 16:33
@jashapiro
Copy link
Member Author

Noting that this is currently targeting development, but if approved, I will cherry pick the changes for a PR to documentation-improvements as well.

@sjspielman
Copy link
Member

Not sure what the future of this PR is, but if we're maybe diving into the css anyways, thought I'd point this out too!

#170 -

I noticed that inline code formatted text looks the same whether it's a link or just regular text. For example, these will have the same styling in readthedocs -

* [`SingleR`](https://bioconductor.org/packages/release/bioc/html/SingleR.html)

* `SingleR`

Maybe we need to incorporate some different styling so in-line code formatted text that is a link clearly appears as a link?

@jashapiro
Copy link
Member Author

Not sure what the future of this PR is, but if we're maybe diving into the css anyways, thought I'd point this out too!

Feel free to file this as a separate issue!

@sjspielman
Copy link
Member

Feel free to file this as a separate issue!

It's filed, I linked the issue! Just trying to intrude to add it to this one, bad Stephanie.

@davidsmejia
Copy link
Contributor

edited the description to add a link to the preview

Copy link
Contributor

@davidsmejia davidsmejia left a comment

Choose a reason for hiding this comment

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

LGTM @dvenprasad gets last say

Copy link
Member

@dvenprasad dvenprasad left a comment

Choose a reason for hiding this comment

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

On board with the wide content space. I think it should be centered for balance

@jashapiro
Copy link
Member Author

On board with the wide content space. I think it should be centered for balance

I kind of agree, but sadly that makes it much harder to pin the version selector box to the bottom of the window. I'm sure the javascript jockeys can make it work, but it can't be done in css. 😢

@dvenprasad
Copy link
Member

The bottom left is a blind spot for majority of users and link RTD isn't an important link. i.e., most people won't notice/be worried about - especially if they are looking for information. I'm not worried about it.

If we want to increase the width for content and center it (with the RTD button hanging out in the corner), we can make that change.

@jashapiro
Copy link
Member Author

Maybe we can restyle the RTD version box so it isn't a such a contrasty grey. I think that would appease my eyes.

@nozomione
Copy link
Member

nozomione commented Nov 10, 2023

On board with the wide content space. I think it should be centered for balance

I kind of agree, but sadly that makes it much harder to pin the version selector box to the bottom of the window. I'm sure the javascript jockeys can make it work, but it can't be done in css. 😢

Hey @jashapiro ! We can accomplish this by using the position sticky to RTD version box (if possible to add style to it) and setting the body width to 1200px only for desktop (we'll need to set the body width to be 100% for mobile), BUT it also adds a scrollbar to body (pls see the screenshot - if we want to hide the scrollbar, then we'll need to edit the site structure).

Just in case I wanted to share this with you for some ideas 💡!

(*I tested this using the developer tool in FF via the link you provided)

body {
  margin: 0 auto;
  position: relative;
  width: 1200px;
}

@media (min-width: 950px) {
   body {
       width: 100%;
    }
}

.rst-versions {
    position: sticky;
    top: calc(100% - 45px);
}

/* make the side nav scrollable when expanded */
.wy-nav-side { 
   background: linear-gradient(0deg,#fdfdfd 0%,#edf7fd 100%);
   max-height: calc(100% - 45px);
   overflow: auto
}
 

Screenshot 2023-11-10 at 5 30 40 PM

@jashapiro
Copy link
Member Author

jashapiro commented Dec 1, 2023

I didn't quite get @nozomione's suggestion to work as it was, but learning about calc() in CSS was the bit I needed! So now this should have what I wanted originally. Just a touch of math in there (Note that I left 1200px/2 rather than 600px for easier future updates: changing all related widths at once is easier if they are all the same!).

I also added an overflow:auto because the file contents submenus were going off screen and I couldn't scroll them.

Should be ready for another look!

@jashapiro jashapiro changed the title Move nav to left and make content wider ~~Move nav to left and~~ make content wider Dec 1, 2023
@jashapiro jashapiro changed the title ~~Move nav to left and~~ make content wider ~Move nav to left and~ make content wider Dec 1, 2023
@jashapiro jashapiro changed the title ~Move nav to left and~ make content wider Make content wider (now back to centered) Dec 1, 2023
Copy link
Member

@dvenprasad dvenprasad left a comment

Choose a reason for hiding this comment

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

Woo looks good!
🦈 🦈 🦈

@jashapiro jashapiro merged commit 26ce43f into development Feb 1, 2024
2 checks passed
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.

5 participants