Move crate drop-down to search results page by jsha · Pull Request #92490 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Relevant to the rustdoc team, which will review and decide on the PR/issue.
label
This comment has been minimized.
This comment has been minimized.
8 tasks
| @@ -1126,9 +1126,16 @@ window.initSearch = function(rawSearchIndex) { |
|---|
| } |
| } |
var output = "Results for " + escape(query.query) + |
| let crates = ``; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't eslint complaining when you use backticks without ${} content?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! I wouldn't expect it to. Backticks do a bunch of things besides template expansion. They also allow multiline strings, and serve as a different delimiter so you don't have to backslash double quotes (\")
As said previously, I really like it! The display needs a bit of improvement: the in is too small I think (but maybe it's voluntary?). Maybe we should force the dropdown to be on its own line on mobile? Currently it gives (with long search string):
The style of the dropdown is incomplete I think: the border is looking strange.
In any case, it's a nice start. :)
the in is too small I think (but maybe it's voluntary?).
This was on purpose, since in ____ isn't part of the title (<h1>). But I could make it the same size as the <h1>, just outside the tag? I don't have a strong opinion about what's better.
Maybe we should force the dropdown to be on its own line on mobile?
Good idea.
I also had an idea recently that could help with this (I forget if I mentioned it or not). We could add a crate:some_crate search syntactical form. Then we could add it as an option to the advanced search builder @jsha proposed.
This was on purpose, since
in ____isn't part of the title (<h1>). But I could make it the same size as the<h1>, just outside the tag? I don't have a strong opinion about what's better.
It looks very strange to me. The select also renders differently from how it used to:
The bezel seems a bit extreme.
Alright, I've increased the size of in and fixed the border issue (I had overzealously removed a border:0 in the CSS that turned out to be needed). I also tweaked the padding slightly. Demo is pushed and ready for another look!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| (query.type ? " (type: " + escape(query.type) + ")" : "") + "" + |
|---|
| "<div id=\"titles\">" + |
| ` in ${crates} ` + |
| "<div id=\"titles\">" + |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think backticks would be better here as well.
| @@ -40,5 +40,5 @@ ENV SCRIPT python3 ../x.py --stage 2 test src/tools/expand-yaml-anchors && \ |
|---|
| /scripts/validate-toolstate.sh && \ |
| /scripts/validate-error-codes.sh && \ |
| # Runs checks to ensure that there are no ES5 issues in our JS code. |
| es-check es5 ../src/librustdoc/html/static/js/*.js && \ |
| es-check es6 ../src/librustdoc/html/static/js/*.js && \ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to make this change in bigger PR but I guess it's fine... Don't forget to update the comment just above please.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think a giant PR adding ES6 features wherever possible, and updating the linter at the same time would be a bad idea. The way we should do it is like I'm doing here:
- update the linter to accept es6 as a standalone commit
- start using es6 features where they are useful
- do a series of standalone commits that do one of two things:
- adopt an ES6 feature where it is useful (e.g. const everything that can be consted), or
- migrate a discrete chunk of code to using various useful ES6 features.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said, it's fine. 😉
Also update Node to v16.9.0, es-check to 6.1.1, and eslint to 8.6.0.
This reduces clutter on doc pages.
📌 Commit 8abb4bb has been approved by GuillaumeGomez
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
…askrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#92055 (Add release notes for 1.58)
- rust-lang#92490 (Move crate drop-down to search results page)
- rust-lang#92510 (Don't resolve blocks in foreign functions)
- rust-lang#92573 (expand: Refactor InvocationCollector visitor for better code reuse)
- rust-lang#92608 (rustdoc: Introduce a resolver cache for sharing data between early doc link resolution and later passes)
- rust-lang#92657 (Implemented const casts of raw pointers)
- rust-lang#92671 (Make
Atomic*::from_mutreturn&mut Atomic*) - rust-lang#92673 (Remove useless collapse toggle on "all items" page)
Failed merges:
r? @ghost
@rustbot modify labels: rollup
Labels
Area: Rustdoc UI (generated HTML)
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the rustdoc team, which will review and decide on the PR/issue.