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.

Sign up for GitHub

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

Jump to bottom

Add unified search options #30311

Merged
merged 2 commits into from
Apr 20, 2022
Merged

Add unified search options #30311

merged 2 commits into from
Apr 20, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Dec 17, 2021 •
edited
Loading

  • Change debounce timeout to 500ms to trigger search less often when typing slowly
  • Allow to set custom value for minimum character limit
  • Allow to set custom value for number of results
  • Add option to disable type ahead functionality and only trigger search on enter/click on the submit button
Peek.2021-12-17.10-15.mp4 
occ config:app:set core unified-search.live-search --value=no
occ config:app:set core unified-search.min-search-length --value=4

@juliusknorr juliusknorr requested review from skjnldsv, a team, artonge and Pytal and removed request for a team December 17, 2021 09:16
@juliusknorr juliusknorr added 3. to review Waiting for reviews enhancement labels Dec 17, 2021
@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Dec 17, 2021
@solracsf solracsf mentioned this pull request Dec 17, 2021
Closed
artonge
artonge reviewed Dec 17, 2021
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Would rename typeAhead to something more explicit, like enableLiveSearch. But else, looks fine :).

PVince81
PVince81 approved these changes Dec 17, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 code looks fine

I agree with @artonge that "type-ahead" sounds more confusing than "live search"

@juliusknorr
Copy link
Member Author

Sure, I'll adjust to live search, thanks for the suggestion.

CarlSchwan
CarlSchwan approved these changes Dec 17, 2021
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Code looks good!

skjnldsv
skjnldsv requested changes Dec 21, 2021
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

minSearchLength is not used? What is it being used for?

jancborchardt
jancborchardt approved these changes Dec 28, 2021
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good design-wise when the settings are customized. :) Also ok on upping the debounce timeout.

@juliusknorr
Copy link
Member Author

minSearchLength is not used? What is it being used for?

It is used in the code already for letting the search only start after a certain amount of characters, just that it was made an option now to configure.

@juliusknorr
Copy link
Member Author

Rebased to latest master

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@blizzz blizzz mentioned this pull request Apr 7, 2022
Merged
@szaimen
Copy link
Contributor

szaimen commented Apr 7, 2022

@juliushaertl what is missing here? :)

@juliusknorr
Increase debounce timeout to be more realistic with real typing behavior
de72801 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Make typeahead and search limit/length configurable
93eb584 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

Just another rebase, pushed and waiting for CI.

@blizzz blizzz mentioned this pull request Apr 13, 2022
Merged
@szaimen
Copy link
Contributor

szaimen commented Apr 13, 2022

acceptance-access-levels is failing. Not sure if related though

@juliusknorr
Copy link
Member Author

Doesn't seem so, but restarted that drone run.

@szaimen
Copy link
Contributor

szaimen commented Apr 19, 2022

Seems to pass now..

@juliusknorr juliusknorr merged commit 8e53a04 into master Apr 20, 2022
@juliusknorr juliusknorr deleted the enh/search-load branch April 20, 2022 14:36
@juliusknorr
Copy link
Member Author

Let's get that in then :)

@szaimen szaimen modified the milestones: Nextcloud 24, Nextcloud 25 Apr 20, 2022
@PVince81
Copy link
Member

@juliushaertl maybe we should backport this also ?

at least for the increased debounce

@juliusknorr
Copy link
Member Author

/backport to stable24

@juliusknorr
Copy link
Member Author

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@juliusknorr
Copy link
Member Author

Will do them manually

@juliusknorr juliusknorr mentioned this pull request Nov 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge left review comments

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@PVince81 PVince81 PVince81 approved these changes

@Pytal Pytal Awaiting requested review from Pytal

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
7 participants
@juliusknorr @szaimen @PVince81 @jancborchardt @artonge @skjnldsv @CarlSchwan