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

Allow unified search filtering and bump @nextcloud/vue #22111

Merged
merged 4 commits into from
Sep 3, 2020
Merged

Allow unified search filtering and bump @nextcloud/vue #22111

merged 4 commits into from
Sep 3, 2020

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 5, 2020 •
edited
Loading

Capture d’écran_2020-08-06_08-34-04

@skjnldsv skjnldsv added this to the Nextcloud 20 milestone Aug 5, 2020
@skjnldsv skjnldsv requested a review from jancborchardt August 5, 2020 10:58
@georgehrke
Copy link
Member

OT: Are there plans to allow search providers to support their own filters?
For events i could think of: before: 2020, at: location123

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 5, 2020 •
edited
Loading

OT: Are there plans to allow search providers to support their own filters?
For events i could think of: before: 2020, at: location123

You can do it already, i'm only filtering out in:xxxx searches, anything else will be passed through the search.

@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the enh/search/make-app-handle-the-order-logic branch from 38621f2 to ea8f68b Compare August 5, 2020 12:52
Base automatically changed from enh/search/make-app-handle-the-order-logic to master August 5, 2020 14:38
@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from 4e687d9 to 6f15bb7 Compare August 5, 2020 14:42
jancborchardt
jancborchardt previously requested changes Aug 5, 2020
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

As discussed, the filters are now a bit too present – more present than the results themselves.

Since it’s basically a guide for very advanced features, they can go into a 3-dot menu on the right inside the search field, which opens a menu with these filters. That way they are not in the way, people can still easily reach them via mouse, and they are also learnable

@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from 3796792 to ee7d2d8 Compare August 6, 2020 06:36
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 6, 2020
@jancborchardt
Copy link
Member

Looks much better with the menu! :) Only: If the icon is the same for all, we don’t need the icon. Or we show the relevant app icon there. :)

@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch 3 times, most recently from d8bd988 to 7cb0022 Compare August 10, 2020 15:02
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 10, 2020
@skjnldsv
Copy link
Member Author

Or we show the relevant app icon there

Would be nice, but we don't provide a unified way to get the apps icons :)
Something for 21? 😁

@nickvergessen
Copy link
Member

But search filters are not bound to apps... So therw is no 1-to-1 mapping to app icons

@skjnldsv

This comment has been minimized.

Sign in to view
georgehrke
georgehrke approved these changes Sep 1, 2020
View reviewed changes
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.

Still working as expected 👍

@georgehrke

This comment has been minimized.

Sign in to view
@skjnldsv

This comment has been minimized.

Sign in to view
@georgehrke

This comment has been minimized.

Sign in to view
@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 1, 2020
@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from 737dfbf to ee35f98 Compare September 1, 2020 13:14
@georgehrke georgehrke mentioned this pull request Sep 1, 2020
Merged
@nickvergessen

This comment has been minimized.

Sign in to view
@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 3, 2020

Rebasing

@nextcloud nextcloud deleted a comment from faily-bot bot Sep 3, 2020
@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from ee3d526 to d80655e Compare September 3, 2020 07:02
@skjnldsv
Allow unified search filtering
f359529 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Fix search placeholder animation & dark theme compatibility
f04b182 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Bump @nextcloud/vue to v2.6.3
43243ce 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from d80655e to ca4c7c3 Compare September 3, 2020 07:03
@danxuliu @skjnldsv
Adjust acceptance tests to changes in @nextcloud/vue 2.6.3
9682194 
The menu button and the menu of a share are no longer direct childs of
the actions of the share row. The menu button is now a child of a
".trigger" element, while the menu is a direct child of the body and has
an id defined in the "aria-describedby" attribute of the ".trigger"
element.

In XPath 1.0 it does not seem possible to "backreference" a value or
create variables, so when the share menu or one of its item is needed
now the ".trigger" element is first found and then its XPath expression
is used to compose its "aria-describedby" attribute in the XPath
expression for the menu.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@skjnldsv skjnldsv force-pushed the enh/unified-search-filters branch from ca4c7c3 to 9682194 Compare September 3, 2020 07:09
nickvergessen
nickvergessen approved these changes Sep 3, 2020
View reviewed changes
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.

🦈

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Sep 3, 2020
@faily-bot
Copy link

faily-bot bot commented Sep 3, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32575: failure

mariadb10.1-php7.2

Show full log 
There were 2 errors:

1) Test\Comments\ManagerTest::testSaveUpdate
InvalidArgumentException: IDs must be translatable to a number in this implementation.

/drone/src/lib/private/Comments/Manager.php:248
/drone/src/lib/private/Comments/Manager.php:809
/drone/src/lib/private/Comments/Manager.php:729
/drone/src/tests/lib/Comments/ManagerTest.php:495

2) OCA\Files_Sharing\Tests\ExpireSharesJobTest::testExpireLinkShare with data set #3 (true, 'P1D', true, false)
Error: Call to a member function getMountPoint() on null

/drone/src/lib/private/Share20/Manager.php:304
/drone/src/lib/private/Share20/Manager.php:705
/drone/src/apps/files_sharing/tests/ExpireSharesJobTest.php:149

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

@skjnldsv skjnldsv merged commit 6a3b649 into master Sep 3, 2020
@skjnldsv skjnldsv deleted the enh/unified-search-filters branch September 3, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgehrke georgehrke georgehrke approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@skjnldsv skjnldsv

@danxuliu danxuliu

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: search
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
5 participants
@skjnldsv @georgehrke @jancborchardt @nickvergessen @danxuliu