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

rustdoc: use flexbox to layout sidebar and main content #89385

Closed
wants to merge 10 commits into from

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Sep 30, 2021

The way rustdoc layouts the sidebar and main content is quite hacky because their content boxes are basically overlapping which eg. causes subpixel anti-aliasing issues on Firefox (with WebRender).

This PR utilizes flexbox and fixes most of the cosmetic issues affecting it (the layout and cosmetics are basically unchanged).

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2021
@rust-log-analyzer

This comment has been minimized.

@cynecx cynecx force-pushed the improve-rustdoc-layout branch from a7df02d to f5425fa Compare September 30, 2021 02:48
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

The source code pages are broken, however it seems like a good idea overall. I'll cc @jsha in here in case I missed something obvious accessibility-wise.

@cynecx
Copy link
Contributor Author

cynecx commented Sep 30, 2021

@GuillaumeGomez My bad. I thought I’ve checked most the sites, but I completely missed the source code view.

@jsha jsha added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Oct 9, 2021
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

I'm excited about this change. Thanks for working on it. One thing I noticed recently looking at source view: there's actually a regular sidebar (hidden), plus a source view sidebar at a different level of the DOM. I think it would be nice to move the source view sidebar up to the same level as the regular sidebar and use the same CSS if we can.

src/librustdoc/html/static/css/rustdoc.css Show resolved Hide resolved
@cynecx cynecx force-pushed the improve-rustdoc-layout branch from f5425fa to 77c1218 Compare October 9, 2021 22:58
@cynecx
Copy link
Contributor Author

cynecx commented Oct 9, 2021

The source page fix was a tricky/hacky one because now the sidebar is basically hidden but the sidebar also has the logo but on the source page we'd probably want to keep the logo, so I've workaround this by duplicating the logo node inside content and showing it only on source pages.

@cynecx
Copy link
Contributor Author

cynecx commented Oct 9, 2021

I think it would be nice to move the source view sidebar up to the same level as the regular sidebar and use the same CSS if we can.

I am not sure if this is a good thing because the source side bar is toggable and overlays the main content. It's basically a "separate" thing, since it is excluded from the main layout (because of position: fixed). It's just easier that way.

@cynecx
Copy link
Contributor Author

cynecx commented Oct 9, 2021

Future work: Rework the search form. It's really hard to work with. We could use display: grid here which would simplify the css/html complexity greatly.

@jsha
Copy link
Contributor

jsha commented Oct 10, 2021

the source side bar is toggable and overlays the main content

FYI in #88301 we concluded we should change that so the source side bar does not overlay the main content.

@GuillaumeGomez
Copy link
Member

Future work: Rework the search form. It's really hard to work with. We could use display: grid here which would simplify the css/html complexity greatly.

We recently encountered huge issues with display: grid so please don't use it.

the source side bar is toggable and overlays the main content

FYI in #88301 we concluded we should change that so the source side bar does not overlay the main content.

Another idea would be to display the source sidebar all the time (not on mobile) and expand it when clicking on a button. You can see what I mean on docs.rs: https://docs.rs/crate/ftml/1.12.3/source/src/lib.rs (I'm quite proud of the usage of display: sticky :D ).

@cynecx
Copy link
Contributor Author

cynecx commented Oct 10, 2021

FYI in #88301 we concluded we should change that so the source side bar does not overlay the main content.

That simplifies things.

We recently encountered huge issues with display: grid so please don't use it.

What kind of issues if I may ask?

Another idea would be to display the source sidebar all the time (not on mobile) and expand it when clicking on a button. You can see what I mean on docs.rs: https://docs.rs/crate/ftml/1.12.3/source/src/lib.rs (I'm quite proud of the usage of display: sticky :D ).

I've merged the source level sidebar into the actual page sidebar. It's shown by default but can be collapsed. On mobile its hidden by default but can be shown by clicking the toggle.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

What kind of issues if I may ask?

This one: #88545

I've merged the source level sidebar into the actual page sidebar. It's shown by default but can be collapsed. On mobile its hidden by default but can be shown by clicking the toggle.

Can you make it collapsed by default please?

@cynecx
Copy link
Contributor Author

cynecx commented Oct 10, 2021

Can you make it collapsed by default please?

Oh I misread. I thought the consensus was to make it visible by default.

@GuillaumeGomez
Copy link
Member

Well, the source sidebar is always visible, but collapsed by default.

@cynecx
Copy link
Contributor Author

cynecx commented Oct 10, 2021

This one: #88545

This issue doesn't seem to affect layouts with a low count of rows or columns. Imho, it doesn't justify a strict hands-off of display: grid, since the benefits of having simpler/lower complexity in css outweighs the potential issues for low row/column count layouts (I was specifically talking about using display: grid for the search form which has like 1-2 rows and 4-5 columns, which shouldn't trigger the bug you linked).

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

This is not looking great when the sidebar is collapsed:

Screenshot from 2021-10-11 11-48-55

But in any case, I had something like this in mind:

Screenshot from 2021-10-11 11-53-56
Screenshot from 2021-10-11 11-54-47

With the file sidebar always visible like on docs.rs. On mobile, I guess the current display is fine.

When scrolling on mobile, the text is moving as well:

Peek.2021-10-11.11-59.mp4

Another problematic thing introduced by this PR: since the logo isn't displayed by default anymore, there isn't an easy way to "go back" to documentation, or at least it's not intuitive to have to open the source sidebar to have access to it. I think it could be solved by displaying the logo at the top as usual when the sidebar is closed and put it into the sidebar when we expand it?

@cynecx
Copy link
Contributor Author

cynecx commented Oct 11, 2021

This is not looking great when the sidebar is collapsed:

Personally, it's just the same way it is now. I don't have a strong opinion on that tbf.

But in any case, I had something like this in mind:

That looks alright too.

On mobile, I guess the current display is fine.

I agree.

I think it could be solved by displaying the logo at the top as usual when the sidebar is closed and put it into the sidebar when we expand it?

I initially thought about this, but it was kinda tricky how to do that with less "hacks" iirc xD.

I don't have much time right now, but I can look into fixing these things later...

@GuillaumeGomez
Copy link
Member

You can move a DOM element using JS (since the sidebar is only available with JS in any case...).

@cynecx
Copy link
Contributor Author

cynecx commented Oct 11, 2021

You can move a DOM element using JS (since the sidebar is only available with JS in any case...).

Yeh sure :D, but I usually try really hard to find a CSS only solution before resorting to JS (to modify the DOM, compared to just modifying a single node's attributes/style).

@GuillaumeGomez
Copy link
Member

Me too but in this case I'm not sure if it's possible... Oh well, if you find a way that'd make me really happy. :)

@apiraino apiraino added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 14, 2021
@jsha jsha added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2021
@cynecx
Copy link
Contributor Author

cynecx commented Oct 24, 2021

@GuillaumeGomez So, I've implemented the "sidebar always visible" approach, and fixed several bugs (sidebar is now sticky and main content doesn't overflow, that way firefox enables sub-pixel antialiasing). The only thing that's missing is the solution/situation about the logo.

EDIT: Added some pics

Desktop

image
image

Mobile

image
image

@rust-log-analyzer

This comment has been minimized.

@cynecx
Copy link
Contributor Author

cynecx commented Oct 24, 2021

The most recent commit also adds the logo next to the controls.

image

image

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 25, 2021

I have a few feedbacks. :)

I'd prefer that the right-border to not be displayed when the sidebar is expanded. Which would look like this:

Screenshot from 2021-10-25 12-01-49

I think it'd be better to remove the transitions when on mobile:

Peek.2021-10-25.12-05.mp4

You didn't hide the rust logo when the sidebar is expanded on desktop:

Screenshot from 2021-10-25 12-07-34

When we are not at the top of the page, the hamburger button disappears on mobile:

Screenshot from 2021-10-25 12-13-43

Finally: please add rustdoc-gui tests for each of the mentioned bugs above. :)

But other than that, it seems all good to me. This will be quite a huge improvement, thanks for working on it!

Also: don't forget to rebase. ;)

@GuillaumeGomez
Copy link
Member

Any update on this @cynecx ?

@cynecx
Copy link
Contributor Author

cynecx commented Nov 9, 2021

@GuillaumeGomez Sorry, I am still caught up in something else :/ I might be able to address your comments at the weekend.

@GuillaumeGomez
Copy link
Member

No problem! Waiting for the next update then. :)

@GuillaumeGomez
Copy link
Member

@cynecx Any news on this? If you don't have time, I'll use your commits and finish it if you don't mind.

@cynecx
Copy link
Contributor Author

cynecx commented Nov 28, 2021

@GuillaumeGomez Sorry, but I don't think I'll get to this within the next 2 weeks.

If you don't have time, I'll use your commits and finish it if you don't mind.

I don't mind at all. That would be great :)

@GuillaumeGomez
Copy link
Member

Closing it in favour of #91356. Thanks again for your work, I had very little to do in the end. 😆

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2021
…, r=jsha

Improve rustdoc layout

This is an overtake of rust-lang#89385 originally written by `@cynecx.`

I kept the original commit and simply added the missing fixes into a new one. You can test it online [here](https://rustdoc.crud.net/imperio/improve-rustdoc-layout/std/index.html).

r? `@jsha`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants