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 search for graphQL schemas #3388

Merged
merged 18 commits into from
Aug 26, 2021

Conversation

Hademumel
Copy link
Contributor

@Hademumel Hademumel commented May 16, 2021

This PR adds search functionality to the graphQL schema explorer.

The search input is visible on the initial schema component and can be jumped to by using a shortcut. The search results are divided into two categories "Types" and "Fields". The number of found elements is thereby limited to 100 per category, and the update function is debounced, in order to allow performant searches in large schemas.

Closes #2720

grafik
grafik

@develohpanda
Copy link
Contributor

Wow, this is awesome! Thanks for opening a PR, added to my list for review. 🙌🏽

In the meantime, since this change involves some UI could you please add screenshots/gifs into the PR description? Thanks!

@Hademumel
Copy link
Contributor Author

Is there nobody who can review this? I implemented this feature cause I want to use it myself 😅

@vercel vercel bot temporarily deployed to Preview July 19, 2021 22:16 Inactive
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Hi @Hademumel! Thank you for this PR and apologies for the delays, we have a lot of work flowing through.

I've just done a preliminary code review. In general this PR looks good, although there are some aspects of the implementation that are a little confusing and worth refactoring - please see my comments below. I have also pushed a commit to resolve conflicts and fix up some type definitions. 😄

As an aside, when you update this PR, could you please make sure to exclude any extra changes that get included as a result of formatting rules in your code-editor (I presume)? It reduces the noise in a code-review and thus helps us to review faster. I updated these for now, but given there will need to be an iteration and further review, please keep it in mind. Thank you! 🙏🏽

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thank you for making those changes! I updated how refs are being used with DebouncedInput to match the existing pattern to simplify (even though the existing pattern is not great, consistency is king). Overall LGTM 🔥 I will hold off on approval until the following questions regarding UX have been addressed from the team.

While testing I noticed that empty states could be better represented. Perhaps with a note under each header (or change the header itself, or not show at all). @wdawson would you have any suggestions on behavior and/or wording?

image

image

Additionally, @erickrico could you please suggest how best to represent/style this concatenation message? This PR will only show 100 results, but if more are found it will present a message like below. Should it be italicized and/or use a fainter text color etc.

image

@wdawson
Copy link
Contributor

wdawson commented Aug 2, 2021

While testing I noticed that empty states could be better represented. Perhaps with a note under each header (or change the header itself, or not show at all). @wdawson would you have any suggestions on behavior and/or wording?

@erickrico may have thoughts here too. But I'd suggest hiding the headers completely if nothing is found for them. If there are no hits across any of the headers, then I'd show some message like "No results found."

@erickrico
Copy link

While testing I noticed that empty states could be better represented. Perhaps with a note under each header (or change the header itself, or not show at all). @wdawson would you have any suggestions on behavior and/or wording?

@erickrico may have thoughts here too. But I'd suggest hiding the headers completely if nothing is found for them. If there are no hits across any of the headers, then I'd show some message like "No results found."

I agree to remove the headers, we should just display something like "no results found, try agin with a different schema", and for the "show 30 more results" @develohpanda i think using a lighter color works best, also what happens when a user click on that, will we load the 30+ results beneath the first batch or results?

@develohpanda
Copy link
Contributor

what happens when a user click on that, will we load the 30+ results beneath the first batch or results?

Currently nothing, that's not a button, just a label. I think the intention is for users to make the search/filter more specific, though it would probably be better if it would show another batch of results 🤔 @erickrico

@gatzjames
Copy link
Contributor

👍 I tested it with a couple of different graphql schemas and it works really well!

@Hademumel
Copy link
Contributor Author

I worked on the proposed changes. Thanks for the suggestions. I used --color-surprise as the color for the "more elements found" hint. Im not sure if this is a good color to use, but I felt it was a good choice to differentiate between the other colors in the schema explorer.

@develohpanda
Copy link
Contributor

@Hademumel thank you for making those updates, much appreciated!

I have pushed a few more commits to improve the TS types (following on from my previous commits). I also pushed an update to replace search with a fuzzy search algorithm Insomnia already uses in other areas, but largely your changes are great! 🙌🏽 I have been testing with various schemas alongside and it seems to be working smoothly. I'm super excited for us to get this through, it's bound to be so useful.

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

Worked well with the GitHub API. Thanks!

Screenshot_20210825_162835

@develohpanda develohpanda enabled auto-merge (squash) August 26, 2021 01:45
@vercel vercel bot temporarily deployed to Preview August 26, 2021 01:45 Inactive
@develohpanda develohpanda merged commit 7135114 into Kong:develop Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation Browser - Supporting Search
6 participants