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

Enh/unified search #21971

Merged
merged 1 commit into from
Aug 3, 2020
Merged

Enh/unified search #21971

merged 1 commit into from
Aug 3, 2020

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jul 23, 2020

a b
Capture d’écran_2020-07-28_16-44-53 Capture d’écran_2020-07-28_16-44-37

API changes

GET https://cloud.domain/search/providers

Now returns a structure like

    [
        { id: 'files', name: 'Files' },
        { id: 'mails', name: 'Mails' },
    ]

Issues

  • Open & focus with ctrl+f
  • Auto-focus first result
  • No results EmptyContent
  • Whole row clickable
  • Open in new tab
  • Shift header menu a bit to the right

Questions

  • Do we need an indicator to display when we reached the end
  • How do we sort the results types? Alphabetically?

@skjnldsv skjnldsv self-assigned this Jul 23, 2020
@skjnldsv skjnldsv added this to the Nextcloud 20 milestone Jul 23, 2020
@skjnldsv skjnldsv force-pushed the enh/unified-search branch from 46dc108 to a4c1fd8 Compare July 23, 2020 15:26
@nickvergessen
Copy link
Member

As for sorting, maybe the current app could be preferred somehow? So in talk Talk results, on files the file results, etc

@jancborchardt
Copy link
Member

jancborchardt commented Jul 25, 2020

@nickvergessen absolutely, good call! :) We even discussed that for apps which don’t have their own in-app search, this way they don’t even have to implement their own search but only hook into the unified search.
(Currently only Talk really has a proper in-app search-as-you-type.)

@raimund-schluessler
Copy link
Member

As for sorting, maybe the current app could be preferred somehow? So in talk Talk results, on files the file results, etc

For the rest of the apps, I would propose the app order as it is shown in the header (but with current app first ofc.).

@skjnldsv
Copy link
Member Author

skjnldsv commented Jul 28, 2020

Issues:

  • First result should open on pressing enter
  • Highlight/bold the part in the results list which is a fit
  • When searching for folder "Earth", instead of subline "/Photos/Space/Earth" say "in Photos/Space" (but still show the whole item path as subline for Comment results)
  • Show folder icons and filetype icons → possibly needs adjustment in the API
  • Similar: Comments needs avatar = round, filetypes rounded off. Possibly directly in API.
  • Show avatar of person for comment results
  • Action icon can go away with whole area being clickable
  • Load more results can be color-text-maxcontrast, and remove arrow
  • Check on accessibility
  • Ellipsize the search input placeholder
  • When you type something, then results show, and the first result is focused – you can not type more without focusing the search field again. Is it possible to do this otherwise, to get the benefit of both having focus on the search input, but that enter would open the first result? (We could not put actual focus on the first element, but simply have CSS styles to highlight the first element, and have JS to open the first element on enter?)
  • When you tab through the list, then tab back up to the search field, and then tab back down, the first entry is skipped. This is probably directly related to the automatic focus of the first result?
  • The emptycontent view says "Start typing to search", better would be "No results for asdf"
  • The filetype icon or preview seems to have alt text? This results in a visual bug where the alt text is shown for a split second. Since it’s purely decorative, the alt text should be empty: alt=""

Unrelated:

  • Ctrl-F doesn’t work with Text enabled

Follow-up

  • Sort results of currently active app first
  • Check the additional filters feature, interface for the "in:files" etc

Stuff for future iterations:

  • Suggestions as emptycontent e.g. pre-filters
  • Ref "sort results of currently active app first" – if it’s well integrated, maybe apps don’t even need their own search in the AppNavigation at all
  • See how unified search relates to the Contacts menu (depending on how we improve the Contacts menu)
  • Alignment further to the right
  • In the case of Files, for a file or folder which is in the current folder we don’t really need to refresh for the scrollto highlight – possible enhancement?

@skjnldsv
Copy link
Member Author

/compile /

@skjnldsv skjnldsv force-pushed the enh/unified-search branch 2 times, most recently from e996f49 to 4116020 Compare July 29, 2020 10:33
@skjnldsv
Copy link
Member Author

Failures seems unrelated!

Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

79AC6EA9-2D66-46D2-A2A4-8064478D21CC

The search icon should remain white even in dark mode.

@nickvergessen
Copy link
Member

When you set the theming color to #FFFFFF and #181818 respectively the section headers are invisible on bright/dark theme.

@georgehrke
Copy link
Member

An icon size of 44x44 pixels looks a bit big for me:
1AD2DAB9-307C-4063-89AF-0C6395D41EE0

@marcelklehr
Copy link
Member

Bookmarks has its own search currently. Any recommendations on how this should be handled?

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the enh/unified-search branch from 4116020 to aefdd64 Compare August 3, 2020 09:23
@faily-bot
Copy link

faily-bot bot commented Aug 3, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 31299: failure

jsunit

Show full log
PhantomJS 2.1.1 (Linux 0.0.0) LOG: 'JQMIGRATE: Migrate is installed, version 1.4.1'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'jQuery is deprecated: The global jQuery is deprecated. It will be updated to v3.x in Nextcloud 21. In later versions of Nextcloud it might be removed completely. Please ship your own.'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: '$ is deprecated: The global jQuery is deprecated. It will be updated to v3.x in Nextcloud 21. In later versions of Nextcloud it might be removed completely. Please ship your own.'
PhantomJS 2.1.1 (Linux 0.0.0) WARN: 'Handlebars is deprecated: please ship your own, this will be removed in Nextcloud 20'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Files.Settings initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Files.Sidebar initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Sharing.ShareSearch initialized'
PhantomJS 2.1.1 (Linux 0.0.0) DEBUG: 'OCA.Sharing.ExternalLinkActions initialized'
PhantomJS 2.1.1 (Linux 0.0.0): Executed 847 of 850 (skipped 3) SUCCESS (11.533 secs / 10.988 secs)

@skjnldsv skjnldsv requested a review from georgehrke August 3, 2020 10:15
@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 3, 2020

Status of 31299: failure

jsunit

curl: (7) Failed to connect to codecov.io port 443: Connection timed out

This is a false negative, we can get this in

@skjnldsv skjnldsv dismissed georgehrke’s stale review August 3, 2020 10:20

Please update again :)

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 3, 2020
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🦈 Bite by bite

Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

Très Bien

@skjnldsv skjnldsv merged commit 4987fe9 into master Aug 3, 2020
@skjnldsv skjnldsv deleted the enh/unified-search branch August 3, 2020 10:43
@skjnldsv skjnldsv added the 4. to release Ready to be released and/or waiting for tests to finish label Aug 3, 2020
@skjnldsv skjnldsv mentioned this pull request Aug 3, 2020
@raimund-schluessler
Copy link
Member

Will there be a migration guide or anything for apps which use the old search already (forgive me if I missed the answer to that)?

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 3, 2020

Will there be a migration guide or anything for apps which use the old search already (forgive me if I missed the answer to that)?

Yes, there is a documentation start in nextcloud/documentation#2166
And I'm planning to extend it right after this is done :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 3, 2020

BTW: this pr got wrongly merged. I failed the rebase it seems
Fix is in #22088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish enhancement feature: search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unified search introduction
6 participants