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

Emit unified search query #22526

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Conversation

raimund-schluessler
Copy link
Member

This PR emits a global event when the unified search is started or reset. Apps can use this to filter their content.

For the Tasks app this looks like this (with nextcloud/tasks#1202):
search

A problem might be that the search is not persistent, as soon as the unified search input is closed, the filtering is reset. But this is by design of the search input, so I guess there is not much to do.

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.

Thanks for taking care of this! 🚀
See comments :)

@skjnldsv
Copy link
Member

skjnldsv commented Sep 1, 2020

A problem might be that the search is not persistent, as soon as the unified search input is closed, the filtering is reset

Let's address that in a followup, nice catch!

@rullzer rullzer mentioned this pull request Sep 1, 2020
21 tasks
@georgehrke
Copy link
Member

D7D33D71-D781-4FCA-BD8A-01CD0FA6A647

@raimund-schluessler Looks like you committed the js files in dev mode. Please compile in production mode.

@raimund-schluessler
Copy link
Member Author

raimund-schluessler commented Sep 1, 2020

@skjnldsv I now wonder whether we should better watch query instead of emitting in the methods. Something like this

watch: {
	query(query, oldQuery) {
		if (query !== oldQuery) {
			debounce(() => {
				emit('nextcloud:unified-search', { query: this.query })
			}, 200)
		}
	},
},

We would be sure to really catch every change of query, even if it will be changed somewhere else in the future.

@raimund-schluessler
Copy link
Member Author

D7D33D71-D781-4FCA-BD8A-01CD0FA6A647

@raimund-schluessler Looks like you committed the js files in dev mode. Please compile in production mode.

Hm, I don't often contribute to server, sorry. I will try to fix this.

@skjnldsv
Copy link
Member

skjnldsv commented Sep 1, 2020

We would be sure to really catch every change of query, even if it will be changed somewhere else in the future.

Well, we are catching any query change. You cannot change it from the outside (not sure we should).
And the only way to trigger it a second time with the same query would be to write some letters and erase them as fast as the current debounce. I'd like to stay clean and avoid watchers as this is not needed.

Unless you encountered some performance/usability issues, but I didn't :)

@raimund-schluessler
Copy link
Member Author

Unless you encountered some performance/usability issues, but I didn't :)

No, usability wise, I think it is ok. I just wondered whether it would be cleaner to only emit at a single place instead of in different methods. But I am fine either way.

@raimund-schluessler raimund-schluessler force-pushed the feature/unified-search/emit-query branch 2 times, most recently from efe6f75 to 88a2ad3 Compare September 1, 2020 20:26
@raimund-schluessler
Copy link
Member Author

I think I fixed it. However, the Node build fails, and I don't know why. It tells me there are uncommited changes. I also see them when I run npm run build locally, but I thought they are unrelated to my changes. Should I also commit them?

@skjnldsv skjnldsv added the 3. to review Waiting for reviews label Sep 2, 2020
@skjnldsv
Copy link
Member

skjnldsv commented Sep 2, 2020

/compile amend /

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@skjnldsv
Copy link
Member

skjnldsv commented Sep 2, 2020

I think I fixed it. However, the Node build fails, and I don't know why. It tells me there are uncommited changes. I also see them when I run npm run build locally, but I thought they are unrelated to my changes. Should I also commit them?

Yes, maybe master have an issue 🤷

@faily-bot
Copy link

faily-bot bot commented Sep 2, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32553: failure

acceptance-header

  • tests/acceptance/features/header.feature:31
Show full log
  Scenario: users from other groups are not seen in the contacts menu when autocompletion is restricted within the same group # /drone/src/tests/acceptance/features/header.feature:31
    Given I am logged in as the admin                                                                                         # LoginPageContext::iAmLoggedInAsTheAdmin()
    And I visit the settings page                                                                                             # SettingsMenuContext::iVisitTheSettingsPage()
    And I open the "Sharing" section of the "Administration" group                                                            # AppNavigationContext::iOpenTheSectionOf()
    And I enable restricting username autocompletion to groups                                                                # SettingsContext::iEnableRestrictingUsernameAutocompletionToGroups()
    And I see that username autocompletion is restricted to groups                                                            # SettingsContext::iSeeThatUsernameAutocompletionIsRestrictedToGroups()
    When I open the Contacts menu                                                                                             # ContactsMenuContext::iOpenTheContactsMenu()
    Then I see that the Contacts menu is shown                                                                                # ContactsMenuContext::iSeeThatTheContactsMenuIsShown()
    And I see that the contact "user0" in the Contacts menu is not shown                                                      # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()
      Failed asserting that true is false.
    And I see that the contact "admin" in the Contacts menu is not shown                                                      # ContactsMenuContext::iSeeThatTheContactInTheContactsMenuIsNotShown()

@raimund-schluessler
Copy link
Member Author

raimund-schluessler commented Sep 2, 2020

I think I fixed it. However, the Node build fails, and I don't know why. It tells me there are uncommited changes. I also see them when I run npm run build locally, but I thought they are unrelated to my changes. Should I also commit them?

Yes, maybe master have an issue 🤷

So, I should commit all files which are generated by npm run build? Or is it alright now?

Edit: I guess compile amend fixed it.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 2, 2020
@skjnldsv skjnldsv merged commit 34aca46 into master Sep 2, 2020
@skjnldsv skjnldsv deleted the feature/unified-search/emit-query branch September 2, 2020 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants