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

Added Name, Description, Column, and SQL filters to search function #108

Merged
merged 5 commits into from
Aug 4, 2020

Conversation

stephen8chang
Copy link
Contributor

@stephen8chang stephen8chang commented Jul 8, 2020

Problem

Currently, the DBT docs search function searches for any instance of a search query that is located in the database, leading to unnecessarily large results pages when searching. Most of the time, users would like the ability to search by a specific criteria, such as the name or description of a node page.

Solution

To allow for more specific search results, this PR adds four new filters that works with the search function: name, description, column, and SQL. Users are now able to narrow down the search results based on their search query and the respective filter they choose.

image

To accomplish this, the following changes were made :

  • SQL code added as something to search for by current search engine
  • Added HTML components for the filter checkboxes next to the search bar
  • filterResults function that takes the current search query and checkboxes clicked to filtered results
  • Text highlighting of SQL code in search results

Other Information

@stephen8chang stephen8chang force-pushed the search-changes-rebase branch from e7d0862 to 3c74c58 Compare July 15, 2020 22:08
@stephen8chang
Copy link
Contributor Author

stephen8chang commented Jul 15, 2020

Just finished adding another component to this feature (shortening the description and SQL text in the search results), and it should now be ready for review.

@drewbanin drewbanin requested review from drewbanin and jtcohen6 July 25, 2020 16:30
@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 2, 2020

This is really cool! Thanks for the contribution @stephen8chang @Matt343, and sorry for the delay in following up. I'm grateful for the thought you've now put into several issues and PRs around improving the docs search functionality.

In playing around with the UX, I find that I want a more flexible system. I think we'd get more mileage out of a text-based search syntax that allows users to specify which property they want to match, along the lines of the issue filtering that GitHub and Gitlab support. That could enable us to build out future filters for all sorts of things: meta properties, table size, etc. It also buys us some time: we can support smarter searches and document the syntax, without taking up visual real estate. Later, we can build out compelling UI elements that recommend available search criteria and values.

A lot of the underlying work in this PR would still apply, but instead of checking boxes, I see users typing name:order, column:customer_id tag:nightly, etc. It does get tricky around logical AND/OR: syntaxes like this tend to return results that match ALL criteria, whereas the approach you've implemented (and I agree) includes results that match ANY criteria. That could be a checkbox IMO, or we could take a page out of the dbt core book and use , to signify intersections.

That said, I'm open to hearing if you feel strongly that a UI-first approach is better here. If you propose sticking to a checkbox-based approach, I have one requested change around design: Could we move the checkboxes to appear below the search bar, and only when the search is focused? I'm imagining:
Screen Shot 2020-08-02 at 1 52 25 PM
That may require relocating the checkboxes from main.html to search.html, and I don't know exactly how that plays with the additions to index.js.

Lastly, there are a few behaviors that make sense, but which may be counterintuitive for users. They're not things we should resolve in this PR; if anything, we need to make separate plans for rationalizing how we search + display information for different resources in the docs site.

  • Description only matches top-level resource description, not nested (column/arg) descriptions.
  • Even though macro names display their package, the package namespace is not technically part of their name, and as such the Name search will not match it. (In the search syntax-based approach, I could imagine making package: an available selector.)

Both of these are true today—more subtly so, because we don't have named filters.

@stephen8chang
Copy link
Contributor Author

@jtcohen6 Thanks for the feedback. As far as the text-based search filter idea goes, I agree that it is a more flexible filter system. However, that will require a whole reworking of this PR, so we would probably make another PR for it; if it's okay with you, I'd like to continue the checkbox filtering system.

On another point, on the topic of moving the checkboxes to below the search bar, there might be a few issues there, mainly due to the fact that the existing filterResults function is inside index.js because of the accessibility to the search query, search results, and other components related to the search function, thus also allowing us to scope some variables in index.js to the HTML components in main.html. If we want to move the checkboxes to search.html, the only way I see around this is to move some of the search-related variables and functions from index.js to search.js. Let me know what you think.

Lastly, in regards to the last two points, those will definitely be acknowledged in future PR's, should we issue any more.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 3, 2020

if it's okay with you, I'd like to continue the checkbox filtering system.

Good by me! I still think this is a great addition, and it's gotten me thinking about the longer-term directions we could head. Thanks for humoring my Sunday afternoon armchair reflection.

If we want to move the checkboxes to search.html, the only way I see around this is to move some of the search-related variables and functions from index.js to search.js

How plausible do you think this would be? From a code-org standpoint, I'd feel good about moving more search logic into search.js, anyway.

Last thing: Given that tags are also a highly-weighted input in #113, what do you think about including a Tags checkbox for this go-round?

@stephen8chang
Copy link
Contributor Author

stephen8chang commented Aug 4, 2020

How plausible do you think this would be? From a code-org standpoint, I'd feel good about moving more search logic into search.js, anyway.

Very possible! I definitely agree that most of the search logic should be in search.js, anyways. I'll get this done and committed as soon as possible.

Last thing: Given that tags are also a highly-weighted input in #113, what do you think about including a Tags checkbox for this go-round?

This is also very possible! I'll add that alongside the other changes above. Let me know what you think. @jtcohen6

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is awesome! Good to go from my end as soon as the merge conflicts are resolved.

There may be small things we'll want to add—explainer text, checkbox styling—but those can come later on.

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.

Add Name, Description, Column, and SQL filters to search function
2 participants