-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Handle notable trait popup differently #91431
Handle notable trait popup differently #91431
Conversation
Some changes occurred in HTML/CSS/JS. |
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
I'd really like to avoid having a second mega JS file that contains stuff for all the crates combined: Can we include the info on the generated file itself? Either as Yes, the current way we generate HTML is nonconformant, but the fix to that is to make it a sibling not a child. |
I'm in favor of the sibling approach too. To be clear: a sibling of the My proposed optimization on the issue was that we'd have a pile of such sibling at the end of each generated HTML page, and select the appropriate one. But that's probably over-optimizing - it's not clear how many pages (other than |
Sorry if I wasn't clear: each file is crate specific. It's not in the root but in the crate (contrary to the search index).
I'm not a big fan of the sibling approach because in the case it's in a
As data attribute could work. It'll just be a very big one haha.
The issue with this is that it might "conflict" with the What about simply putting it at the end of the DOM?
I think it's the best approach because it'd be much simpler to handle. Sibling with |
Thinking about it more, sibling of the summary tag won't work well if we have multiple of these. It's kinda unfortunate that we're putting the entire signature inside the A way to still have HTML work would be to generate them all at the end and give them ids. Not great. I'm still not a fan of generating these at runtime since it moves more generation logic into JS, but I'm not sure if this is any better. |
It can be pure HTML at the end of the current file too, doesn't have to be pure JS. But in any case, the display will need to be handled by JS unfortunately. |
There's a crucial distinction: If it's HTML at the end of the file, JS never needs to construct HTML, it simply needs to toggle some on-element CSS properties. It's the constructing-HTML-in-JS that I wish to avoid. |
I'm fine with both approach. What do you think @jsha? Just to sum it up:
|
Yep, that's an accurate summary. I'm not a fan of either approach but I have a slight preference to the HTML one from a code architecture point of view. |
@@ -129,6 +129,9 @@ crate struct SharedContext<'tcx> { | |||
crate cache: Cache, | |||
|
|||
crate call_locations: AllCallLocations, | |||
/// The key is the notable trait text and the value is its index (we add it in the DOM so the | |||
/// JS knows which notable trait to pick). | |||
crate notable_traits: RefCell<FxHashMap<(String, String), usize>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you absolutely sure this needs a RefCell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now at least it does unfortunately...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't notice this is in the SharedContext.
0d2839e
to
6a6044c
Compare
I moved the DOM directly into the item's page. So now the JS only display it (in the correct location). |
This comment has been minimized.
This comment has been minimized.
6a6044c
to
8a3604c
Compare
This comment has been minimized.
This comment has been minimized.
8a3604c
to
e9edbf1
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #91957) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing this after a discussion with the author |
POC for #91290.
So this is what I had in mind: having a
notable-traits.js
file in each crate and load it only when needed (I replicated what @jsha did for the search).Currently, it's only on click but we can also add "mouseover", I didn't do it in this POC for simplicity. To handle the click, I simply changed the current code. When clicking on "i", we move and update the popup to display the notable trait information.
The advantage of this method is that we have element to care about and that's it and our HTML is finally valid based on the HTML spec.
You can try it out here.
Ah also, size of the rendering before/after:
So not much difference.
cc @jsha @Manishearth