Skip to content

Conversation

nbdd0121
Copy link
Member

@nbdd0121 nbdd0121 commented Sep 6, 2021

This ensures that sidebar does not overlap with content when horizontal scrollbar is present.

The overlap is visible in https://docs.rs/ndarray/0.15.3/ndarray/struct.ArrayBase.html.

Discovered by @m13253.

@rust-highfive
Copy link
Contributor

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Contributor

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 6, 2021
@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Member Author

nbdd0121 commented Sep 6, 2021

The error doesn't seem relevant...

@GuillaumeGomez
Copy link
Member

I don't think this is the right fix. The content shouldn't be larger that the container. The scrollbar should be on the <table> directly.

@nbdd0121
Copy link
Member Author

nbdd0121 commented Sep 7, 2021

The scrollbar should be on the <table> directly.

Many elements could cause the overflow, not just a table. E.g. a very long inline code block could cause the content to overflow too, but you cannot put a scrollbar on it because it isn't and shouldn't be display: block.

For this particular case a scrollbar on the table might be desirable, but in general style still shouldn't break when content overflows.

@GuillaumeGomez
Copy link
Member

Agreed. I'm planning to wrap the <table> items into <div> and to add this CSS rule:

.docblock * {
    max-width: 100%;
    overflow-x: scroll;
}

That should prevent most issues.

@nbdd0121
Copy link
Member Author

nbdd0121 commented Sep 7, 2021

Should be overflow-x: auto;

This ensures that sidebar does not overlap with content when horizontal
scrollbar is present.
@GuillaumeGomez
Copy link
Member

I opened #88742 so I think this PR can be closed. To give a few more information about why I don't think this is a good idea: hiding the issue (but making the scrolled content go under the sidebar) doesn't fix the original problem. Instead of making the whole doc block scroll, I prefer to limit it to the element having a too big width directly.

Thanks in any case for bringing this issue to our attention! :)

@nbdd0121
Copy link
Member Author

nbdd0121 commented Sep 8, 2021

No I think both PRs should be merged. They tackle different problems:

  • Fix table in docblocks #88742 reduces the chance of overflow happening;
  • This PR addresses the behaviour when overflow happens: even if overflow happens, the result should still be sane.

As I said earlier, it not feasible in general to prevent overflow from happening at all. Have

`<insert a very long symbol name here>` Note that's an inline code block so it doesn't make sense to put a scroll bar on it.

in the doc still shouldn't break the style and cause the overlap to happen.

@nbdd0121
Copy link
Member Author

nbdd0121 commented Sep 8, 2021

Oh I miss the .docblock > * { max-width: 100%; } part. I suppose that'll work too.

@nbdd0121 nbdd0121 closed this Sep 8, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 10, 2021
…ks, r=nbdd0121

Fix table in docblocks

"Overwrite" of rust-lang#88702.

Instead of adding a z-index to the sidebar (which only hides the issue, doesn't fix it), I wrap `<table>` elements inside a `<div>` and limit all chidren of `.docblock` elements' width to prevent having the scrollbar on the whole doc block.

![Screenshot from 2021-09-08 15-11-24](https://user-images.githubusercontent.com/3050060/132515740-71796515-e74f-429f-ba98-2596bdbf781c.png)

Thanks `@nbdd0121` for `overflow-x: auto;`. ;)

r? `@notriddle`
@nbdd0121 nbdd0121 deleted the css-is-hard branch September 11, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants