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 users and apps inner search and add HeaderMenu cancel #22800

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Sep 11, 2020

  • Add back search with the following UX flow:
    • Searching within the Unified Search also filters inner content
    • Pressing escape just closes the header menu and does NOT reset the search
    • Clicking outside just closes the header menu and does NOT reset the search
    • Pressing the reset search button clears everything AND reset the unified search state

This was requested by @jancborchardt and should be the default behaviour for other apps.


Alongside I added a cancel event to the HeaderMenu component that could be use by other apps once I moved it to https://github.com/nextcloud/nextcloud-vue

@skjnldsv
Copy link
Member Author

/compile amend /

@raimund-schluessler
Copy link
Member

@skjnldsv I guess this fixes the issue discussed in #22526 (comment), right?

@raimund-schluessler
Copy link
Member

  • Pressing the reset search button clears everything AND reset the unified search state

I can not see any reset search button. Where is it supposed to be?

Screenshot_2020-09-11 Inventar - Nextcloud

@raimund-schluessler
Copy link
Member

It would be nice if it could be somehow indicated when a search is still "active", i.e. the search string is not empty. Maybe with a red dot or at least the opacity of the search icon could be set to 1.

@skjnldsv
Copy link
Member Author

It would be nice if it could be somehow indicated when a search is still "active", i.e. the search string is not empty. Maybe with a red dot or at least the opacity of the search icon could be set to 1.

why?

I can not see any reset search button. Where is it supposed to be?

What browser?
Capture d’écran_2020-09-12_11-24-33

@raimund-schluessler
Copy link
Member

What browser?
Capture d’écran_2020-09-12_11-24-33

This is with latest Firefox 80.0.1. In Chrome is indeed visible.

@skjnldsv
Copy link
Member Author

I'll add a dedicated reset button for firefox.
Thanks Mozilla...
image

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 12, 2020
@raimund-schluessler
Copy link
Member

It would be nice if it could be somehow indicated when a search is still "active", i.e. the search string is not empty. Maybe with a red dot or at least the opacity of the search icon could be set to 1.

why?

Because it is currently not obvious when a search/filter is active and users might be confused why certain content is not shown.

No search:
Screenshot_2020-09-12 Tasks - Nextcloud(1)

With filter/search:
Screenshot_2020-09-12 Tasks - Nextcloud

@rullzer rullzer mentioned this pull request Sep 14, 2020
5 tasks
@jancborchardt
Copy link
Member

Because it is currently not obvious when a search/filter is active and users might be confused why certain content is not shown.

Good point … this is unfortunately where the drawback of showing the search field only in the dropdown comes in. The way we did it before with the search field being directly in the header, this was obvious.

  • So ideally, we could move again to that, showing the search field part of the search in the header. The results popover can be directly below. This is also how it’s commonly done in other apps like Google products, as well as on Android for example.
  • Or as a step inbetween, when the app does inline filtering and you click somewhere else and the popover closes, the popover doesn’t fully close but the search field still shows?
  • Or we really do clear the search when the popover closes, although that is unexpected and would be quite frustrating.

@skjnldsv
Copy link
Member Author

  • So ideally, we could move again to that, showing the search field part of the search in the header. The results popover can be directly below. This is also how it’s commonly done in other apps like Google products, as well as on Android for example.

The issue we had was on small screens. But I'm fine if you prefer than. Though I would be also fine just adding an indicator like the white triangle under? or a red dot like notifications? 🤔 😀

  • Or as a step inbetween, when the app does inline filtering and you click somewhere else and the popover closes, the popover doesn’t fully close but the search field still shows?

I'm afraid it could conflict with other elements on page, especially mobile

  • Or we really do clear the search when the popover closes, although that is unexpected and would be quite frustrating.

You asked not to, so we're good there, no clearing on close :)

@jancborchardt
Copy link
Member

The issue we had was on small screens. But I'm fine if you prefer than. Though I would be also fine just adding an indicator like the white triangle under? or a red dot like notifications?

So the search would initially be an icon, but as soon as you click or focus, it would expand (like before).

On mobile, it’s fine to do it like on Android: Search can take the full heading width (at least on smartphone sizes) and go back to icon-only when you clear the search. (On tablet or intermediate sizes, it’s fine if search takes as much space as it needs away from the list of app icons.)

An indicator on the search icon is something that can easily be missed and also something I haven’t seen elsewhere before.

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

@raimund-schluessler this will wait for21, Jan have a bit more in-depth requests for this that will create too much changes for 20 :)

@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 14, 2020
@raimund-schluessler
Copy link
Member

@raimund-schluessler this will wait for21, Jan have a bit more in-depth requests for this that will create too much changes for 20 :)

@skjnldsv What exactly will have to wait? The whole PR or indicating that a search is active?
Maybe as a compromise before not doing anything we could at least increase the opacity of the search icon to 1 when the search is active.

@raimund-schluessler
Copy link
Member

And adding a reset button for Firefox is not controversial as well, right?

@skjnldsv
Copy link
Member Author

Maybe as a compromise before not doing anything we could at least increase the opacity of the search icon to 1 when the search is active.

  • Or as a step inbetween, when the app does inline filtering and you click somewhere else and the popover closes, the popover doesn’t fully close but the search field still shows?

This will be implemented in a followup for 20

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke

This comment has been minimized.

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 bug high regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants