Skip to content

Better result dom generation #85540

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
May 29, 2021

Conversation

GuillaumeGomez
Copy link
Member

First commit is from #85506.

We realized in #85506 (comment) thanks to @dns2utf8 that in some cases, the generated search result DOM was invalid. This was not strict enough and the DOM was inserted as a big string, which wasn't great.

r? @jsha

@rust-highfive
Copy link
Contributor

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 21, 2021
@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-js Area: Rustdoc's JS front-end labels May 21, 2021
@dns2utf8
Copy link
Contributor

dns2utf8 commented May 21, 2021

I found that some entries are not visible for some reason, but I can select them if I go down with the arrow keys and press enter, for example:

MyWs::fn send_to_client(&mut self, msg: ClientCommand, ctx: &mut WebsocketContext<Self>)

https://data.estada.ch/rustdoc-nightly_4e3e6db01_2021-05-18/multiplayer_snake/index.html?search=send
updated with the current search.js

Update: link to the correct search term

@dns2utf8
Copy link
Contributor

I noticed when I looked at it with Chromium that the entries that are missing don't have a description text:
grafik

I am still in a meeting, but I can take a look at it later

@GuillaumeGomez
Copy link
Member Author

Ok, I found the issue. That was a tricky one: it lacks a div when there is no short documentation. The items are there, just not in the right place. Talk about tricky case. Great catch!

@dns2utf8
Copy link
Contributor

Ah yes, and with the float: left on the entry it collapses the whole link to height: 0px 😄

@dns2utf8
Copy link
Contributor

dns2utf8 commented May 21, 2021

I made an error in one of my comments in the other thread: it should have been 00a0 (nbsp) instead of 00ad (shy) then it works with Firefox too.
Very sorry about that

@GuillaumeGomez
Copy link
Member Author

@dns2utf8 No problem, I'll update the char at the same time as the rest. :)

name + "</span>";
wrapper.appendChild(resultName);

var description = document.createElement("div");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var description = document.createElement("div");
var description = document.createElement("div");
description.className = "result-description";

Adding this would simplify the CSS in the mobile variant

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do it in another PR, this one is big enough, I prefer style changes to be put on their own.

@bors
Copy link
Collaborator

bors commented May 21, 2021

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

@GuillaumeGomez
Copy link
Member Author

#85551 slightly changed the DOM generation by moving the desc class on the <span> parent so that will need to be done here too once it's merged.

@GuillaumeGomez GuillaumeGomez force-pushed the better-result-dom-generation branch from 77cc3c9 to 0fae87a Compare May 25, 2021 15:51
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 25, 2021

Updated!

I also realized that some CSS rules weren't updated so I did and I added a test for them.

@GuillaumeGomez
Copy link
Member Author

ping @jsha

@jsha
Copy link
Contributor

jsha commented May 29, 2021

@bors r+

We should figure out a better long-term plan for generating HTML in JS - like <template> elements. Manually generating the DOM with createElement calls like this quickly becomes unreadable.

@bors
Copy link
Collaborator

bors commented May 29, 2021

📌 Commit 0fae87a has been approved by jsha

@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 May 29, 2021
@bors
Copy link
Collaborator

bors commented May 29, 2021

⌛ Testing commit 0fae87a with merge ff5522f...

@dns2utf8
Copy link
Contributor

I like the templates idea. (Btw. the scroll-margin bug I found is now documented)

@bors
Copy link
Collaborator

bors commented May 29, 2021

☀️ Test successful - checks-actions
Approved by: jsha
Pushing ff5522f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2021
@bors bors merged commit ff5522f into rust-lang:master May 29, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 29, 2021
@GuillaumeGomez GuillaumeGomez deleted the better-result-dom-generation branch May 29, 2021 19:55
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 24, 2021
…Gomez

rustdoc: staggered layout for module contents on mobile

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.

* Demo: https://data.estada.ch/rustdoc-nightly_126561cb3_2021-05-25/generic_array/index.html
* Related: rust-lang#85540

## Desktop
![grafik](https://user-images.githubusercontent.com/739070/119416896-2be62300-bce4-11eb-9555-792b859ab611.png)

## Mobile
![grafik](https://user-images.githubusercontent.com/739070/119416934-44563d80-bce4-11eb-9e77-70a72edcc487.png)

r? `@GuillaumeGomez` `@jsha`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-rustdoc-js Area: Rustdoc's JS front-end 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. 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