4

rustdoc: staggered layout for module contents on mobile by dns2utf8 · Pull Reque...

 3 years ago
source link: https://github.com/rust-lang/rust/pull/85651
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

New issue

rustdoc: staggered layout for module contents on mobile #85651

Conversation

Copy link

Contributor

dns2utf8 commented on May 24

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

Mobile

r? @GuillaumeGomez @jsha

Copy link

Collaborator

rust-highfive commented on May 24

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

This comment has been hidden.

Copy link

Contributor

jsha commented on May 25

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.

Copy link

Contributor

jsha commented on May 25

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.

Copy link

Contributor

Author

dns2utf8 commented on May 29

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?

Copy link

Contributor

Author

dns2utf8 commented on May 29

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

Copy link

Member

GuillaumeGomez commented on May 29

I hope it will. :)

This comment has been hidden.

jsha

changed the title Staggered layout with rustdoc on mobile

rustdoc: staggered layout for module contents on mobile

on May 30

@@ -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 {

jsha on May 30

Contributor

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.

jsha 14 days ago

Contributor

Bump on this comment

Copy link

Contributor

jsha commented on May 30

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.

Copy link

Contributor

jsha commented on May 30

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?

This comment has been hidden.

Copy link

Contributor

Author

dns2utf8 commented on May 30

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?

Copy link

Contributor

jsha commented on May 30

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

Copy link

Contributor

Author

dns2utf8 commented on May 30

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

Copy link

Contributor

jsha commented on May 30

Is your intent for item-table / item-left / item-right to be used elsewhere in the markup, or just for the listing of items in the module?

If the former, I'd give this item-table a class "module-items" and change the XPath selector to //item-table[class=module-items]/item-right.

If the latter I'd do the same selector but without the class.

Copy link

Contributor

bors commented 28 days ago

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

This comment has been hidden.

Copy link

Contributor

Author

dns2utf8 commented 15 days ago

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.

Copy link

Contributor

Author

dns2utf8 commented 15 days ago

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

This comment has been hidden.

Copy link

Contributor

Author

dns2utf8 commented 15 days ago

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/

This comment has been hidden.

Copy link

Member

GuillaumeGomez commented 7 days ago

@bors retry

Copy link

Contributor

bors commented 7 days ago

hourglass Testing commit b5610fb with merge eab335e...

This comment has been hidden.

Copy link

Member

GuillaumeGomez commented 7 days ago

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

Any info about this @pietroalbini ?

Copy link

Contributor

Author

dns2utf8 commented 7 days ago

I rebased the PR on master again, hope this helps

Copy link

Member

GuillaumeGomez commented 7 days ago

Waiting for CI to confirm then approving again.

Copy link

Collaborator

rust-log-analyzer commented 7 days ago

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

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

Copy link

Contributor

bors commented 6 days ago

pushpin Commit 94c84bd has been approved by GuillaumeGomez

Copy link

Contributor

bors commented 6 days ago

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

Copy link

Contributor

bors commented 6 days ago

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

bors

merged commit 7c3872e into rust-lang:master 6 days ago

11 checks passed

rustbot

added this to the 1.55.0 milestone

6 days ago

Copy link

Contributor

Author

dns2utf8 commented 6 days ago

It merged partying_face

Copy link

Contributor

jsha commented 6 days ago

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

Copy link

Contributor

Author

dns2utf8 commented 6 days ago

Thank you for supporting it!

dns2utf8

deleted the

dns2utf8:rustdoc_flexbox

branch

6 days ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects

None yet

Milestone

1.55.0

Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK