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

New UI global search #40823

Merged
merged 5 commits into from
Nov 10, 2023
Merged

New UI global search #40823

merged 5 commits into from
Nov 10, 2023

Conversation

nfebe
Copy link
Contributor

@nfebe nfebe commented Oct 9, 2023

Resolves: #39162

@nfebe nfebe force-pushed the 39162-global-search-2.0 branch 8 times, most recently from 3439b86 to 1259830 Compare October 14, 2023 22:36
@nfebe nfebe added this to the Nextcloud 28 milestone Oct 15, 2023
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch 2 times, most recently from 2c653fe to d1e8f04 Compare October 17, 2023 07:56
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.

Quick code quality review, (while I know this is a very early state 😉 )

  • console.xxxxx should be avoided and a logger used instead
  • Tabs not spaces
  • Please format your classes using the BEM format
  • Please also document the various sections in your templates with html comments

I won't do a review of the code itself as I assume this will heavily change in the future :)

@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from d1e8f04 to 1d78e71 Compare October 24, 2023 20:43
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@nextcloud nextcloud deleted a comment from netlify bot Oct 25, 2023
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from 1d78e71 to 90ba51a Compare October 26, 2023 09:27
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
apps/files/lib/Search/FilesSearchProvider.php Fixed Show fixed Hide fixed
lib/private/Search/FilterCollection.php Fixed Show fixed Hide fixed
apps/dav/lib/CardDAV/CardDavBackend.php Fixed Show fixed Hide fixed
lib/public/Search/IFilterCollection.php Fixed Show fixed Hide fixed
lib/public/Search/IFilterCollection.php Fixed Show fixed Hide fixed
lib/private/Search/FilterFactory.php Fixed Show fixed Hide fixed
apps/files/lib/Search/FilesSearchProvider.php Fixed Show fixed Hide fixed
core/Controller/UnifiedSearchController.php Fixed Show fixed Hide fixed
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch 2 times, most recently from 98993c7 to be176b0 Compare October 26, 2023 21:34
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from be176b0 to 8b78df6 Compare November 2, 2023 16:11
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch 3 times, most recently from 6b09c96 to 2b30563 Compare November 3, 2023 18:05
@blizzz blizzz mentioned this pull request Nov 6, 2023
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch 2 times, most recently from 69e9518 to 1a28f92 Compare November 9, 2023 09:21
@blizzz
Copy link
Member

blizzz commented Nov 9, 2023

Not up to date with main branch. I guess it needs another rebase and compile 🤔

No, there are no conflicts on the compiled assets

@blizzz blizzz added the 4. to release Ready to be released and/or waiting for tests to finish label Nov 9, 2023
@blizzz
Copy link
Member

blizzz commented Nov 9, 2023

CI complains however:

ERROR: There is 1 use of triple dot instead of ellipsis:
At line 17 in file /drone/src/build/../core/src/views/GlobalSearchModal.vue

Does it refer to https://github.com/nextcloud/server/pull/40823/files#diff-72e59d550e6509240d3c7bad01689aa5d0f9ce1c3b530fb7a45100fedebf37ffR18 ? Should be replaced with … .

P.S.: There are also apparent "real" test failures in https://drone.nextcloud.com/nextcloud/server/43505/9/4

@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from a33cc34 to cd24eac Compare November 9, 2023 23:06
@nfebe
Copy link
Contributor Author

nfebe commented Nov 9, 2023

/compile amend /

@marcoambrosini
Copy link
Member

@fenn-cs I pushed the searchable list component for people. To add it to global search modal copy-paste this:

<!-- People search popover -->
    <SearchableList :label-text="t('core', 'Search people')"
	    :search-list="peopleSeclectProps.options"
	    :empty-content-text="t('core', 'Not found')">
	    <template #trigger>
		    <NcButton>
			    {{ t('core', 'People') }}
		    </NcButton>
	    </template>
    </SearchableList>

With a few tweaks, we should be able to easily adapt this for the apps too

@AndyScherzinger
Copy link
Member

/compile amend /

@Altahrim Altahrim force-pushed the 39162-global-search-2.0 branch from 29c0389 to 060ef94 Compare November 10, 2023 09:45
@Altahrim
Copy link
Collaborator

Cypress failure unrelated (already in master)

nfebe and others added 3 commits November 10, 2023 11:27
We are introducing a new search UI that providers a lot more space
for users via a large centralized modal and providers various filters
which can by applied by adding various chips on the UI.

For example, users can now filter their search or scope it by limiting the results
to specific apps, time period and people by apply the appropriate filters on the
new UI, previously filters where applied using text in the search box by prefixing
with `::`.

Resolves: #39162

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
The new global search UI is new and might be unstable, hence
we are giving users the option to use the old unified search UI, if
the encounter signficant bottlenecks.

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Signed-off-by: Marco <marcoambrosini@icloud.com>
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from 060ef94 to 4f4cec2 Compare November 10, 2023 10:28
This commit migrates away from NcSelect which has a couple of
accesibility and display problems currently, hence a new component
`SearchableList` is now used.

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the 39162-global-search-2.0 branch from 4f4cec2 to 360a372 Compare November 10, 2023 10:33
@nfebe
Copy link
Contributor Author

nfebe commented Nov 10, 2023

/compile amend /

@nfebe
Copy link
Contributor Author

nfebe commented Nov 10, 2023

/compile /

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nfebe nfebe merged commit 95e5642 into master Nov 10, 2023
49 of 51 checks passed
@nfebe nfebe deleted the 39162-global-search-2.0 branch November 10, 2023 11:40
@nfebe
Copy link
Contributor Author

nfebe commented Nov 10, 2023

Bringing this down here, so it remains visible and easily transferable to a new ticket.

TODO

  • Icon inversion which does not work well now since icons come in white or black instead of one consistent color. (Black icons turn white and white icons turn black, leading to to white on white background issues)
  • Use the new people filter (selector) that @marcoambrosini is working on (Current people filter not completed) - DONE, however, NcPopover which is used has issues with closing triggers.
  • Update back-end API (if ability not already present) to show "Search in App" dynamically.

From @ChristophWurst

BUG: Clicking outside the search modal needs two clicks on the magnifier icon to get the modal back.
BUG: Some app icons are white on white.
BUG: Modified timestamp bubbles change size on hover, have no pointing cursor.
BUG: Generally no feedback for no results.
BUG: Search people has white text on white background.
BUG: Search people has a tiny user bubble, barely wider than the avatar.
BUG: Search people No result goes to the top, overlaps other input.
BUG: Search people has no effect at all. The search string doesn't contain a person identifier.

@ChristophWurst
Copy link
Member

And all of #40823 (comment)

@nfebe
Copy link
Contributor Author

nfebe commented Nov 10, 2023

Thanks @ChristophWurst updated.

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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🔎 Advanced search
8 participants