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: staggered layout for module contents on mobile #85651

Merged
merged 4 commits into from
Jun 24, 2021

Conversation

dns2utf8
Copy link
Contributor

This PR adds the container <item-table> with its two children <item-left> and <item-right>.
It uses grid-layout on desktop and flexbox on mobile to make better use of the available space.

Additionally it allows to share parts of the CSS with the search function.

Desktop

grafik

Mobile

grafik

r? @GuillaumeGomez @jsha

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

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

This comment has been minimized.

@jsha
Copy link
Contributor

jsha commented May 25, 2021

Note that the item names (e.g. GenericArray) are supposed to be in sans-serif. See #85621, where we recently fixed this by restoring a CSS rule that applied sans-serif to links in tables. I think you'll want to replace that CSS rule with one that applies sans-serif to this new layout. Probably it would be best to give these items a class of their own, and apply the CSS to that class.

@jsha
Copy link
Contributor

jsha commented May 25, 2021

Also, we might need a little vertical margin between items to visually separate them. I can't tell from the screenshots because they only include sections that have a single item each.

@dns2utf8
Copy link
Contributor Author

Thanks for the hint! Currently it looks exactly the same (except for 1px more vertical space between the table item and the description and the serif links you mentioned) as the table variant, I added a bit of vertical space already.

My plan is to rebase on master next week and demo with a bigger crate than genneric_array, which I choose because it uses almost all rustdoc features.
If you have a better suited crate I will render that one too :)

Another question: currently the element names item-table, item-left and item-right are very expressive. I could shorten them and potentially reduce the size of the rendered HTML. I guesstimate that the current choice consumes about the same space as the table variant, but we could save a little bit if we wanted.
What do you think?

@dns2utf8
Copy link
Contributor Author

PS: I am also playing with the thought of merging #85540 into this PR, or will that one be merged soon anyways?

@GuillaumeGomez
Copy link
Member

I hope it will. :)

@dns2utf8
Copy link
Contributor Author

I uploaded an interactive example: https://data.estada.ch/rustdoc-nightly_2023cc3aa1_2021-05-30/rustdoc_demo/

Desktop

grafik

Mobile

grafik

@rust-log-analyzer

This comment has been minimized.

@jsha jsha changed the title Staggered layout with rustdoc on mobile rustdoc: staggered layout for module contents on mobile May 30, 2021
@@ -1756,7 +1786,8 @@ details.undocumented[open] > summary::before {
.search-results .result-name, .search-results div.desc, .search-results .result-description {
width: 100%;
}
.search-results div.desc, .search-results .result-description {
/* Display second row of staggered layouts */
.search-results div.desc, .search-results .result-description, item-right {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about the "ideal" spacing some more. For the staggered layout in search, I suggested 2em out of the blue. It would be nice for consistency to use a spacing that we use elsewhere. For instance, for docblocks we indent them 24px relative to their heading:

.docblock {
	margin-left: 24px;
	position: relative;
}

That works out to about 1.5em at our font sizes. So I think we should change padding-left: 2em to margin-left: 24px with a comment that we are matching it to the docblock indent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump on this comment

@jsha
Copy link
Contributor

jsha commented May 30, 2021

When I access your demo, I get this message in the Firefox console:

Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-2TDOnxRwDtZllEt9eciuRNF6fRBZiYRlTWKiW4H0dK0='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

It probably doesn't make a difference for this particular PR but you might want to exempt your demo paths from that CSP rule, since it could make a difference in rendering.

@jsha
Copy link
Contributor

jsha commented May 30, 2021

Other than my comment about margin-left, and the test failures, this looks good to me. @GuillaumeGomez do you want to give feedback before merge or can I r+ once the tests are fixed?

@rust-log-analyzer

This comment has been minimized.

@dns2utf8
Copy link
Contributor Author

I tried to fix one of the tests, but it looks like the feature following-sibling:: in the used XPATH implementation does not know about that. Can I add that or does anybody have another suggestion?

@jsha
Copy link
Contributor

jsha commented May 30, 2021

Why do you need following-sibling? Does this work? // @has foo/index.html '//*[@class="module-item"]//item-right[@class="docblock-short"]' ""

@dns2utf8
Copy link
Contributor Author

I need it because with grid-layout the structure changes from table -> tr* -> [ td , td ] to item-table -> [ item-left, item-right ]* and the item-left has the module-item class

@jsha
Copy link
Contributor

jsha commented May 30, 2021 via email

@bors
Copy link
Contributor

bors commented Jun 3, 2021

☔ The latest upstream changes (presumably #84703) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@dns2utf8
Copy link
Contributor Author

I did plan to use it as a more generic way for all the static tables.
We can not use it for the search results for now, because we rely on the :hover class and for that we would need a grouping element which is not compatible with grid layout.

So for now, I am going with the item path without the class.

@dns2utf8
Copy link
Contributor Author

PS: I am using the grid layout because it automatically compacts the first column which would require active JavaScript with flexbox

@rust-log-analyzer

This comment has been minimized.

@dns2utf8
Copy link
Contributor Author

I fixed all the tests but there is a gap that is untestable:

  • with the current python parser we can not verify sibling relationships
  • only top down selectors are supported

There appears to be an alternative: lxml
However, the compatibility page indicates that we might have to put some work into a migration.
What do you think?

Apart from that, I think this PR is pretty much ready please review the demo here because optically it is pretty much the same as before:
https://data.estada.ch/rustdoc-nightly_2336406b38_2021-06-16/rustdoc_demo/

@rust-log-analyzer

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2021
@bors
Copy link
Contributor

bors commented Jun 24, 2021

⌛ Testing commit b5610fbdae367f395899a40a4e92fd84e4d2d34c with merge eab335e32ed4fc5fa3a5df653fc439d040bcc568...

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

The CI really doesn't want this PR (or is very broken).

Any info about this @pietroalbini ?

@dns2utf8
Copy link
Contributor Author

I rebased the PR on master again, hope this helps

@GuillaumeGomez
Copy link
Member

Waiting for CI to confirm then approving again.

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@GuillaumeGomez
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 24, 2021

📌 Commit 94c84bd has been approved by GuillaumeGomez

@bors
Copy link
Contributor

bors commented Jun 24, 2021

⌛ Testing commit 94c84bd with merge 7c3872e...

@bors
Copy link
Contributor

bors commented Jun 24, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 7c3872e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 24, 2021
@bors bors merged commit 7c3872e into rust-lang:master Jun 24, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 24, 2021
@dns2utf8
Copy link
Contributor Author

It merged 🥳

@jsha
Copy link
Contributor

jsha commented Jun 24, 2021

Congrats! Thanks for sticking through it. This will be a very nice improvement.

@dns2utf8
Copy link
Contributor Author

Thank you for supporting it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants