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

Add tabs for search for better information access #45055

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
6 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 5, 2017

A few screenshots:

screen shot 2017-10-06 at 00 54 51

screen shot 2017-10-06 at 00 54 58

screen shot 2017-10-06 at 00 55 00

r? @rust-lang/docs

cc @killercup @QuietMisdreavus

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Oct 5, 2017

[00:04:01] tidy error: /checkout/src/librustdoc/html/static/main.js:896: line longer than 100 chars
[00:04:01] tidy error: /checkout/src/librustdoc/html/static/main.js:898: line longer than 100 chars
[00:04:01] some tidy checks failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Oct 6, 2017

Arf, forgot tidy check once again...

@killercup
Copy link
Member

killercup left a comment

Nice! This is a very nice step to making the search more accessible.

At RustFest, we talked a bit about what the search can already do, but one thing I didn't think of: Can you search for a function that has "foo" in its name that returns a usize? I'm not sure I have a good idea how to do the UI for this, but I guess the code itself might already support this in some capacity.

@@ -342,6 +342,18 @@
}
}

function findArg(obj, val) {
if (!obj || !obj.type || obj.type.inputs.length === 0) {

This comment has been minimized.

@killercup

killercup Oct 6, 2017

Member

Heh, that's such a negative way to write this ;) How about

obj && obj.type && obj.type.inputs && obj.type.inputs.length > 0

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 6, 2017

Author Member

I wanted to avoid putting all my code in an if condition but either way is fine for me so let's go for it!

return true;
}
}
return false;

This comment has been minimized.

@killercup

killercup Oct 6, 2017

Member

You replace the whole for thing with

return obj.type.inputs.some(function (x) { return x.name === name; })

but I'm not sure if we still need to support browsers which don't have Array.prototype.some.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 6, 2017

Author Member

That's the point. I'm supporting as much browsers as I can.

'<div id="titles">' +
'<div class="selected">Types/modules</div>' +
'<div>As parameters</div>' +
'<div>Returned</div></div><div id="results">';

This comment has been minimized.

@killercup

killercup Oct 6, 2017

Member

Maybe

As return value

for consistency?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 6, 2017

Author Member

Good point!

(query.type ? ' (type: ' + escape(query.type) + ')' : '') + '</h1>' +
'<div id="titles">' +
'<div class="selected">Types/modules</div>' +
'<div>As parameters</div>' +

This comment has been minimized.

@killercup

killercup Oct 6, 2017

Member

Maybe

As a parameter

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 6, 2017

Author Member

You can look for multiple parameters at once (String, usize -> *).

var elems = document.getElementById('titles').childNodes;
elems[0].onclick = function() { printTab(0); };
elems[1].onclick = function() { printTab(1); };
elems[2].onclick = function() { printTab(2); };

This comment has been minimized.

@killercup

killercup Oct 6, 2017

Member

Protip:

elems[2].onclick = function() { printTab(2); };

can be

elems[2].onclick = printTab.bind(null, 2)

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 6, 2017

Author Member

I find it less readable. :-/

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:search-tabs branch 2 times, most recently from dbff8b7 to 52bcc43 Oct 6, 2017

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Oct 6, 2017

At RustFest, we talked a bit about what the search can already do, but one thing I didn't think of: Can you search for a function that has "foo" in its name that returns a usize? I'm not sure I have a good idea how to do the UI for this, but I guess the code itself might already support this in some capacity.

You can't do both at once. You can say that you're looking for a function called to_string:

fn: to_string

Or for a function which returns usize:

fn: -> usize
if (results['others'].length < maxResults) {
results['others'].push(obj);
}
} else {

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Oct 8, 2017

Member

Forgive my ignorance of the JS part of rustdoc, but i think i'm missing something. What is obj.type here, and why does its presence keep it from getting into results['others']? Is that on purpose?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 8, 2017

Author Member

You can see everything as a hashmap in js, so obj.type == obj['type']. In this case, if there is no type (which is just a regular field, nothing particular), it means it's not a function nor a method so I put it in "others".

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Oct 8, 2017

Member

So is results['others'] not meant to be the same as the current search results? I'm concerned that you won't be able to find a function by name if it's not returning something with the same name. For example, are you able to find str::to_lowercase by searching for something like lowercase? Will you be able to find that method on any tab in your new layout?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:search-tabs branch 2 times, most recently from 1cb08c0 to 8a53447 Oct 9, 2017

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:search-tabs branch from 8a53447 to 3a65d12 Oct 9, 2017

}
}
if (results['others'].length < maxResults &&
((query.search && obj.name.indexOf(query.search)) || added === false)) {

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Oct 9, 2017

Member

Why is this condition not just results['others'].length < maxResults? I just checked out your PR and didn't see any difference between the longer form you have here and taking these extra conditions out. What is this trying to do?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 10, 2017

Author Member

In case the only thing(s ?) match are the arguments or the returned type, I don't want to add extra information. For me, the 'others' tab is also about the type/function/module's name so it seems logical to not add everything (even if the filter is pretty light).

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Oct 10, 2017

Member

After some checking, it seems like it won't stop things getting into results if their name matches as well as having a parameter type that matches (e.g. "Result" will show Try::into_result on both the "As return value" tab as well as the "Types/modules" tab) so i'll retract this concern.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Oct 10, 2017

@bors r+

I'm incredibly excited to see this land. I think it greatly helps discoverability in docs. It's not quite perfect (e.g. searches for io things won't get a lot of type-based results since they're all wrapped up in Results, but searching for Result doesn't return the mass of responses you would expect - in fact, it only returns places where Result was used with generic types, like on the Try trait, or on Option/Result combinators) but i'd like to get this landed and work on tweaking the search index in follow-up PRs.

cc #44024 since this helps that out

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 10, 2017

📌 Commit 3a65d12 has been approved by QuietMisdreavus

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 11, 2017

⌛️ Testing commit 3a65d12 with merge 8dcd0e1...

bors added a commit that referenced this pull request Oct 11, 2017

Auto merge of #45055 - GuillaumeGomez:search-tabs, r=QuietMisdreavus
Add tabs for search for better information access

A few screenshots:

<img width="1440" alt="screen shot 2017-10-06 at 00 54 51" src="https://user-images.githubusercontent.com/3050060/31256148-032c1a06-aa31-11e7-8e4c-fec59786b8e6.png">
<img width="1440" alt="screen shot 2017-10-06 at 00 54 58" src="https://user-images.githubusercontent.com/3050060/31256150-03312cb2-aa31-11e7-86f7-8c9f0d8d6d4f.png">
<img width="1440" alt="screen shot 2017-10-06 at 00 55 00" src="https://user-images.githubusercontent.com/3050060/31256149-0330d456-aa31-11e7-8f89-3b3c824e30b4.png">

r? @rust-lang/docs

cc @killercup @QuietMisdreavus
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 11, 2017

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Oct 11, 2017

@bors retry

  • Android SDK HTTPS issue (fixed in #45193)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2017

⌛️ Testing commit 3a65d12 with merge d21c023...

bors added a commit that referenced this pull request Oct 13, 2017

Auto merge of #45055 - GuillaumeGomez:search-tabs, r=QuietMisdreavus
Add tabs for search for better information access

A few screenshots:

<img width="1440" alt="screen shot 2017-10-06 at 00 54 51" src="https://user-images.githubusercontent.com/3050060/31256148-032c1a06-aa31-11e7-8e4c-fec59786b8e6.png">
<img width="1440" alt="screen shot 2017-10-06 at 00 54 58" src="https://user-images.githubusercontent.com/3050060/31256150-03312cb2-aa31-11e7-86f7-8c9f0d8d6d4f.png">
<img width="1440" alt="screen shot 2017-10-06 at 00 55 00" src="https://user-images.githubusercontent.com/3050060/31256149-0330d456-aa31-11e7-8f89-3b3c824e30b4.png">

r? @rust-lang/docs

cc @killercup @QuietMisdreavus
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing d21c023 to master...

@bors bors merged commit 3a65d12 into rust-lang:master Oct 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:search-tabs branch Oct 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.