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

Search in Doc Explorer #80

Merged
merged 1 commit into from
Jan 22, 2016
Merged

Search in Doc Explorer #80

merged 1 commit into from
Jan 22, 2016

Conversation

asiandrummer
Copy link
Contributor

Search functionality in Documentation Explorer.
It's pretty basic with searching/getting results/clicking links - I'll incrementally add in more details in upcoming PRs.

EDIT: added no-search-result case and minimum character length

@@ -45,8 +45,8 @@ export class DocExplorer extends React.Component {
// Public API
showDoc(typeOrField) {
var navStack = this.state.navStack;
var isCurrentlyShown =
navStack.length > 0 && navStack[navStack.length - 1] === typeOrField;
var isCurrentlyShown = navStack.length > 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unnecessary change - reverting.

@asiandrummer
Copy link
Contributor Author

screen shot 2016-01-15 at 4 17 58 pm

screen shot 2016-01-15 at 4 17 50 pm

@asiandrummer
Copy link
Contributor Author

screen shot 2016-01-15 at 4 43 39 pm

screen shot 2016-01-15 at 4 39 30 pm

this.state = { navStack: [] };
this.state = {
navStack: [],
searchValue: null
Copy link
Contributor

Choose a reason for hiding this comment

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

I think searchValue should probably be part of the nav stack rather than it's own thing, so that you can search, click on something, click back and be where you were before.

This could be done in a separate PR though, since it's a bit beyond what's happening here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly; I would like the search action to be a part of navStack - something like:
navStack = [
testType: { ... },
graphiql_search: { value: ... },
];

Copy link
Contributor

Choose a reason for hiding this comment

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

A related note about navStack for the future: it would be cool if scroll position were also maintained as part of the back stack. Today if you click from a long scrolling view to another long scrolling view your scroll position is maintained when it should probably be reset to "0", but then when going "back" the original scroll position should be restored. It's a fairly small detail, but it will help make navigating larger schema easier on the mind.

@leebyron
Copy link
Contributor

This is awesome! There are some comments inline, but this is pretty close to landing as an initial version of search.

@asiandrummer
Copy link
Contributor Author

Responded to comments:

  • removed ch restriction for now
  • got rid of the argument indexing, turns out we didn't need it after all
  • removed specific match types and included wholesome within search results
  • a field with multiple argument matches

@asiandrummer
Copy link
Contributor Author

screen shot 2016-01-21 at 2 56 40 pm

@@ -103,12 +114,21 @@ export class DocExplorer extends React.Component {
field={typeOrField}
onClickType={this._onClickTypeOrField}
/>;
} else if (this.state.searchValue) {
title = 'Search Results';
content = <SearchDoc
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use the same styling here as in the else block below, line break and indent.

@leebyron
Copy link
Contributor

This looks great. I have a few other comments in there. One overarching minor thing: it's nice to use const instead of let as the default for defining variables, and use let as the exception when you need to reassign it in the future.

Once you've covered all the comments and updated it, merge away!

let matches = field.args.filter(arg => {
return arg.name.indexOf(searchValue) !== -1;
});
matchedFields.push(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will allow search results to match arguments/render fields every time. Fixed:

if (matches.length > 0) {

@asiandrummer
Copy link
Contributor Author

Thanks @leebyron!
Next diffs will include navStack integration and a newer format in Doc Explorer.

asiandrummer added a commit that referenced this pull request Jan 22, 2016
@asiandrummer asiandrummer merged commit ad5881d into master Jan 22, 2016
@asiandrummer asiandrummer deleted the search-doc-explorer branch March 9, 2016 23:51
acao pushed a commit to acao/graphiql that referenced this pull request Jun 5, 2019
…ransform-es2015-spread-6.22.0

Update babel-plugin-transform-es2015-spread to version 6.22.0 🚀
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants