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 app-based filtering to global search #41669

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

Fenn-CS
Copy link
Contributor

@Fenn-CS Fenn-CS commented Nov 22, 2023

Emit search queries from the global search modal that would trigger list filtering in various apps that support it.

@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Nov 22, 2023
@Fenn-CS Fenn-CS mentioned this pull request Nov 23, 2023
5 tasks
@Fenn-CS Fenn-CS force-pushed the app-based-filtering-global-search branch 2 times, most recently from 82577ee to b430abb Compare November 23, 2023 10:46
@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Nov 23, 2023

/compile amend /

@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@AndyScherzinger
Copy link
Member

/backport to stable28

@AndyScherzinger
Copy link
Member

/compile amend /

Emit search queries from the global search modal that would trigger
list filtering in various apps that support it.

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliushaertl
Copy link
Member

From a UX perspective i feel this is the wrong approach to filter the app from within the modal, so I hope #41609 is also considered soonish.

@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Nov 23, 2023

From a UX perspective i feel this is the wrong approach to filter the app from within the modal, so I hope #41609 is also considered soonish.

Well, I have already over-stated this point, especially after implementing and interacting with this. The priority communicated is that we first restore the "original" behavior which is what this PR is about.

So I hope, we undo this asap and improve on what Marco has proposed already. I think even in its current state that's way more decent and at least we have the toggle for users to just go back to the old search.

@@ -227,6 +238,9 @@ export default {
this.results = []
return
}
if (this.supportFiltering()) {
emit('nextcloud:unified-search.search', { query })
Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, why restrict?
If the app doesn't subscribe to the event, it will not do anything right?

Copy link
Contributor Author

@Fenn-CS Fenn-CS Nov 23, 2023

Choose a reason for hiding this comment

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

  1. For the very reason you stated, emitting it does not do anything so, no need to emit it when it's not needed.
  2. Code clarity, it makes it clear why, where and when the event needs to be emitted, otherwise it could pass that it is just another step in the search procedure

Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to emit it when it's not needed.

I think we don't know that. Other applications out there could use this mechanism.

Copy link
Member

@juliushaertl juliushaertl Nov 23, 2023

Choose a reason for hiding this comment

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

Yep, would agree, especially for community apps we don't maintain or developers working on that I'd vote for always emitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would open a follow up PR then!

@AndyScherzinger AndyScherzinger merged commit 953382e into master Nov 23, 2023
40 of 42 checks passed
@AndyScherzinger AndyScherzinger deleted the app-based-filtering-global-search branch November 23, 2023 15:51
@Fenn-CS
Copy link
Contributor Author

Fenn-CS commented Nov 26, 2023

This partially solves #41484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants