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 keyword search to filter example queries #83

Merged

Conversation

IoannisNezis
Copy link
Collaborator

@IoannisNezis IoannisNezis commented Mar 6, 2024

This PR adds a keyword-search to filter the example querys (by name).

before after
20240306_18h35m47s_grim 20240306_17h53m32s_grim

Each whitespace sperated word needs to appear in the result.

The search matches the keyword also infix
20240306_18h12m52s_grim

When no result matches the query, a message is displayed.
20240306_18h41m10s_grim

@IoannisNezis IoannisNezis self-assigned this Mar 6, 2024
@IoannisNezis IoannisNezis added the enhancement New feature or request label Mar 6, 2024
@IoannisNezis IoannisNezis requested a review from hannahbast March 7, 2024 15:06
@IoannisNezis
Copy link
Collaborator Author

Maybe debouncing the search would be nice

@IoannisNezis
Copy link
Collaborator Author

IoannisNezis commented Mar 23, 2024

I played around a bit with different highlighting options:

background underline
20240323_19h02m39s_grim 20240323_19h04m02s_grim

I personally prefer the underline option, any opinions?

@hannahbast
Copy link
Member

@IoannisNezis This is awesome! It's live on https://qlever.cs.uni-freiburg.de for some time now and I use it for almost every example query. The underlining looks great. A minor suggestion for further improvement: When one clicks on the Example button, the focus should be in the search field so that one can start typing right away without first clicking into the search field

Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

Looks great, a few minor suggestions. Using match(regex) for the search is not a must, but maybe you like it.

@@ -0,0 +1,76 @@
// this function removes the highlight
Copy link
Member

Choose a reason for hiding this comment

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

A minor comment: Our comment style is normal sentences (capitalized and with a full stop). I know that a lot of the qlever-ui does not follow this rule yet. That's becase it's old. But nor reason not to stick to this for all new code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good to know!
I generated some docstrings with chatGPT.
Do the new comments satisfy the comment style?

/**
 * Removes highlighting from the input string by replacing all occurrences of
 * <span> elements with the class "keyword-search-highlight" with their inner content.
 *
 * @param {string} input_str - The input string possibly containing highlighted spans.
 * @returns {string} - The input string with highlighting removed.
 */

Copy link
Member

Choose a reason for hiding this comment

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

We use ordinary // ... comments with full sentences. When referring to variable or class names in a comment, we put it in .... See the code on https://github.com/ad-freiburg/qlever for many examples of this (it's also not 100% consistent there, but 90% of the comments follow this convention).

backend/static/js/keywordSearch.js Outdated Show resolved Hide resolved
backend/static/js/keywordSearch.js Outdated Show resolved Hide resolved
backend/templates/index.html Outdated Show resolved Hide resolved
@hannahbast hannahbast changed the title added keyword search to filter example querys Add keyword search to filter example queries Mar 27, 2024
@hannahbast
Copy link
Member

@IoannisNezis This looks great now.

Another small request, which occurred to me already when trying this out the first time and still occurs to me now every time I used the new feature:

When selecting a query using the "Example" button one has to:

  1. Press the Example button
  2. Start typing some word fragments (thanks to your latest change, a click in the search field is no longer necessary)
  3. Click on the desired query
  4. Execute the query

For Step 4, there is the Shortut Ctrl+Return, which I always use to execute a query (after you type the query, you can just continue typing in instead of having to switch to the mouse again just for a single click).

But with the workflow above, I have to click, type, click, click

It would be awesome if one could select the query with either TAB or arrow-up / arrow down and then select it via Return (and another Ctrl+Return would then execute it). Interestingly, when pressing TAB once, one already gets to "Add new example", and then Return takes you to the corresponding page. So maybe realizing this with TAB is relatively little additional work.

@IoannisNezis
Copy link
Collaborator Author

IoannisNezis commented Mar 29, 2024

@hannahbast
I actually already started working on this, initially this is a bit of a pain.
I did not look into the tabindex direction yet, thanks for the idea.

At this point it might be worth mentioning that i am re-implementing parts of this library: https://select2.org/ which i initially choose not to use.

If there is a good reason not to use this library i would be really happy (sunken cost fallacy is kicking in really badly).
One reason might be: being able to search for multiple keywords of one example.

@IoannisNezis
Copy link
Collaborator Author

I implemented the keyboard control.
It should now be possible to search, select and execute an example query while only clicking once.

I decided against the tabulator option and implemented the selection with arrow keys.

Further improvements

I want to implement 2 further improvements:

Scroll

The list of possible examples is a bit long.
i want to make the list scrollable, instead of showing everything.

Exclusive Hover

Currently one can select a example with the mouse and the arrow keys.
I want to change this so that if one hovers an example only this example is highlighted.
If the arrow key is used after a element is hovered with the mouse it should propagate from this element.

Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

1-1 with Ioannis, this looks great, just some minor changes

@IoannisNezis
Copy link
Collaborator Author

@hannahbast
In the last 2 commits I implemented the formatting changes we discussed (typo, line-length, ...).

A few things:

  1. What is the procedure for closing threads in PR's?
    Should only the reviewer close them or can the assignee also close threads if its reasonable?

  2. In a previous comment i suggested two further improvements.
    I would like to address them in separate PR's to keep this one at a manageable size.

Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

Finally finishing this review and merging this. Thanks for the extremely useful addition, Ioannis!

@hannahbast hannahbast merged commit 273f129 into ad-freiburg:master Aug 6, 2024
IoannisNezis added a commit to IoannisNezis/qlever-ui that referenced this pull request Sep 1, 2024
* master:
  Don't ignore `db` when building Docker image (ad-freiburg#101)
  Update deprecated setting for static storage (ad-freiburg#95)
  Build and push Docker image after each commit to the master (ad-freiburg#98)
  Update db/qleverui.sqlite3 to latest configs and example queries
  Display details for operation in "Analysis" tree as tooltip (ad-freiburg#80)
  Bump dependencies `requests`, `gunicorn`, `django` to latest version
  Improvements in Docker setup (ad-freiburg#90)
  Add keyword search to filter example queries (ad-freiburg#83)
  Restore default qleverui.sqlite3
  Fix regex in replacePredicatesList
  No subject AC query for short prefix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants