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.

Sign up for GitHub

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

Jump to bottom

Move the sidebar to be stuck to left of the content #999

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Move the sidebar to be stuck to left of the content #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51a0f13
Move the sidebar to be stuck to left of the content
EmileTrotignon Sep 14, 2023
51baa29
changes
EmileTrotignon Nov 13, 2023
9ac5b54
add comment
EmileTrotignon Dec 5, 2023
c027a90
support files
EmileTrotignon Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ Tags:

### Changed
- Style: Remove font fallback to Helvetica, Arial (@Julow, #1028)
- Style : Sidebar is now stuck to the left of the content instead of the left
of the viewport (@EmileTrotignon, #999)

### Changed
- Style: Adjusted line height in the TOC to improve readability (@sorawee, #1045)
Expand Down
94 changes: 67 additions & 27 deletions src/html_support_files/odoc.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,28 +263,52 @@ body {
}

body {
margin-left: calc(10vw + 20ex);
margin-right: 4ex;
margin-top: 30px;
margin-bottom: 50px;
margin-left: auto;
margin-right: auto;
padding: 0 4ex;
}

body.odoc {
max-width: 100ex;
max-width: 132ex;
display: grid;
grid-template-columns: min-content 1fr;
column-gap: 4ex;
row-gap: 2ex;
}

body.odoc-src {
margin-right: calc(10vw + 20ex);
}

.odoc-content {
grid-row: 4;
grid-column: 2;
}

.odoc-preamble > *:first-child {
/* This make the first thing in the preamble align with the sidebar */
padding-top: 0;
margin-top: 0;
}

header {
margin-bottom: 30px;
}

header.odoc-preamble {
grid-column: 2;
grid-row: 3;
}

nav {
font-family: "Fira Sans", sans-serif;
}

nav.odoc-nav {
grid-column: 2;
grid-row: 2;
}

/* Basic markup elements */

b, strong {
Expand Down Expand Up @@ -480,7 +504,7 @@ h4 {
font-size: 1.12em;
}

/* Comment delimiters, hidden but accessible to screen readers and
/* Comment delimiters, hidden but accessible to screen readers and
selected for copy/pasting */

/* Taken from bootstrap */
Expand Down Expand Up @@ -759,19 +783,32 @@ td.def-doc *:first-child {
line-height: 1.2;
}

/* When a search bar is present, we need the sticky sidebar to be a bit lower,
so `top` is higher */

.odoc-search + * + .odoc-toc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment saying that this is to accommodate the sidebar would be useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is done

--toc-top: calc(var(--search-bar-height) + var(--search-padding-top) + 20px);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the search bar sticky as well ? If not, this is not needed at all, --toc-top: 20px is sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately the search bar is sticky and I find that not satisfying. The search bar takes valuable vertical space and is not properly spaced from the content is hovers. It's only surrounded by a white shadow that makes it hard to spot where the contents begins and doesn't fit with Odoc's theme.

When the search bar is not present, this calculation adds space above the TOC that doesn't look good either.

I suggest making the search bar non sticky to fix all of that.

Copy link
Collaborator Author

@EmileTrotignon EmileTrotignon Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is not properly spaced from the content is hovers

i do not understand this part

When the search bar is not present, this calculation adds space above the TOC that doesn't look good
either.

I can fix this.

It's only surrounded by a white shadow that makes it hard to spot where the contents begins

This is probably fixable, although I do not remember having this issue

doesn't fit with Odoc's theme.

Its true the input element is practically unstyled, but I plan to style it in my colors pr. As of now, it is dependent on browser defaults, which are very different and sometimes ugly.

I like the sticky sidebar, for the reason that I think that searching for something while looking at the doc is very useful. If it is not sticky, if you read the doc and the want to search for something, then you have to scroll to the top, and might forget what you wanted to do, and you also cannot type the query while looking at the doc (for instance copying a typename in a type-search query).
Also, the sidebar was not made sticky by this PR, and changing it is a little out of scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is not properly spaced from the content is hovers

I mean that it's hard to distinguish where the content starts below the search bar.

If it is not sticky, if you read the doc and the want to search for something, then you have to scroll to the top, and might forget what you wanted to do, and you also cannot type the query while looking at the doc

I don't think this workflow is well implemented currently. The dropdown showing the search results is above the page and might interfere. On mobile, almost none of the page is visible while typing the search query.
This waste vertical space at all time, not just when I intent to search.

Also, the sidebar was not made sticky by this PR, and changing it is a little out of scope.

Agree! I just noticed that the search bar was sticky while reviewing this calculation. Let's not do change the search bar in this PR.

Copy link
Collaborator Author

@EmileTrotignon EmileTrotignon Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On mobile, almost none of the page is visible while typing the search query.

True, the search bar should not be sticky on mobile

This waste vertical space at all time, not just when I intent to search.

That is true, there is a tradeoff here. Maybe with some js magic we can have both, but I am not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would trade having to scroll to the top for more vertical space.

max-height: calc(100vh - 2 * var(--toc-top));
top: var(--toc-top)
}

.odoc-toc {
position: fixed;
top: 0px;
bottom: 0px;
left: 0px;
max-width: 30ex;
min-width: 26ex;
width: 20%;
--toc-top: 20px;
width: 28ex;
background: var(--toc-background);
overflow: auto;
color: var(--toc-color);
padding-left: 2ex;
padding-right: 2ex;
grid-row-start: 3;
grid-row-end: 5;
grid-column: 1;
height: fit-content;
border: solid 1px var(--border);
border-radius: 5px;
position:sticky;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sticky is interesting but it has a behavior I find confusing:
If the side bar is longer than the screen is high but is smaller than the content, it's not possible to see the bottom of the side bar until we reach the bottom of the page.

Is here a way to make the side bar sticky only if the bottom of it is visible ?
Otherwise, I think I slightly prefer having a unsticky side bar. It's much more intuitive.

Note: We might make the side bar longer in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is here a way to make the side bar sticky only if the bottom of it is visible ?

I agree that would be nice. It is certainly possible with javascript. I will look for a css only solution.
I think the sticky is important because the bar is use for navigation inside the page itself.

Copy link
Collaborator

@panglesd panglesd Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another solution is to have a maximal height for the left bar of the size of height, and add a scroll bar.

However, I do not love any of the solution!

  • Sticky, bottom of long nav bar inaccessible unless at the bottom of the page is a "no" for me
  • "Custom stickiness", with some js solving the above problem: why not, but not very satisfying to have to use JS.
  • Not sticky: simple and works, but not the best since we quickly lose the table of content in long pages

I'll be ok with those last two solutions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no perfect solution and I think the scrollable side bar is the best compromise of usability and simplicity.

max-height: calc(100vh - 2 * var(--toc-top));
top: var(--toc-top)
}

.odoc-toc ul li a {
Expand All @@ -789,26 +826,31 @@ td.def-doc *:first-child {
}

:root {
--search-bar-height: 20px;
--search-bar-height: 25px;
--search-padding-top: 1rem;
}

.odoc-search {
--padding-top: 1rem;
position: sticky;
top: 0;
background: var(--main-background);
height: calc(var(--search-bar-height) + var(--padding-top));
/* This amounts to fit-content when the search is not active, but when you
have the search results displayed, you do not want the height of the search
container to change. */
height: calc(var(--search-bar-height) + var(--search-padding-top));
width: 100%;
padding-top: var(--padding-top);
padding-top: var(--search-padding-top);
z-index: 1;
grid-row: 1;
grid-column-start: 1;
grid-column-end: 3;
}


.odoc-search .search-inner {
width: 100%;
position: relative;
left: 0;
transition: left 0.3s, transform 0.3s, width 0.3s;
display: grid;
/* The second column is for the search snake, which has 0 width */
grid-template-columns: 1fr 0fr;
Expand All @@ -818,20 +860,13 @@ td.def-doc *:first-child {
background: transparent;
}

.odoc-search:focus-within .search-inner {
/* Search inner is bigger than its parent, but the overflow needs to be
centered. */
left: 50%;
transform: translateX(-50%);
width: 110%;
}

.odoc-search .search-bar {
position: relative;
z-index: 2;
font-size: 1em;
transition: font-size 0.3s;
box-shadow: 0px 0px 0.2rem 0.3em var(--main-background);
height: var(--search-bar-height);
}

.odoc-search:focus-within .search-bar {
Expand Down Expand Up @@ -934,7 +969,7 @@ td.def-doc *:first-child {
.odoc-search .search-entry {
color: var(--color);
display: grid;
/* Possible kinds are the following :
/* Possible kinds are the following :
"doc" "type" "mod" "exn" "class" "meth" "cons" "sig" "cons" "field" "val"
and "ext".
As the longest is 5 characters (and the font monospace), we give 5
Expand Down Expand Up @@ -1113,6 +1148,11 @@ td.def-doc *:first-child {
@media only screen and (max-width: 110ex) {
body {
margin: 2em;
padding: 0;
}

body.odoc {
display: block;
}

.odoc-toc {
Expand Down
Loading
Loading