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 multiselect type to ActionInput #1250

Merged
merged 2 commits into from
Jan 18, 2021
Merged

Conversation

korelstar
Copy link
Contributor

@korelstar korelstar commented Aug 2, 2020

Straight forward implementation: simply passes all attributes and events from/to multiselect component.

Closes #774

Preview of documentation: https://deploy-preview-1250--nextcloud-vue-components.netlify.app/#/Components/Actions?id=actioninput

@korelstar korelstar added 3. to review Waiting for reviews feature: actions Related to the actions components labels Aug 2, 2020
@korelstar korelstar requested a review from skjnldsv August 2, 2020 09:22
Copy link
Contributor

@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.

You need to implement the focusable class trick so that accessibility works :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 2, 2020

Other than this! Great addition!! 👍

@skjnldsv skjnldsv added the feature: select Related to the NcSelect* components label Aug 2, 2020
@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 2, 2020

You need to implement the focusable class trick so that accessibility works :)

Ah, it doesn't seems that you can :/
https://github.com/shentao/vue-multiselect/blob/d8d29e7714449f4fca4bfe10d3bd21e72c46c9b7/src/Multiselect.vue#L47

@korelstar
Copy link
Contributor Author

Added the focusable class and focusing the ActionInput element using cursor keys works now.

@korelstar korelstar requested a review from skjnldsv August 2, 2020 12:24
@@ -68,6 +68,7 @@ For the multiselect component, all events will be passed through. Please see the
:value="value"
:placeholder="text"
:disabled="disabled"
:class="{ focusable: isFocusable }"
Copy link
Contributor

Choose a reason for hiding this comment

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

It won't work, it needs to be on the input so that the browser canfocus the proper element :/

Copy link
Contributor Author

@korelstar korelstar Aug 2, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot select anything inside.
It properly opens it, but it's not fully working :)

But I don't think it's an issue with the component itself in the end, but more like a keydown event conflict. Once it's focused, we should prevent anything from opening.
The datepicker should not open on focus. Maybe just let the user enter a date or something. 🤷
And the multiselect is a tricky one too. Maybe have the search directly set to true so that keyboard users can directly find what they're looking for?
I don't know 🤷

@korelstar
Copy link
Contributor Author

@skjnldsv what can we do in order to get this in? Honestly, I don't know how to fix the issue you have raised. Moreover, the issue existed already before for the datepicker. So, what should we do here?

@skjnldsv
Copy link
Contributor

Honestly, I don't know how to fix the issue you have raised. Moreover, the issue existed already before for the datepicker. So, what should we do here?

#1250 (comment)
Honestly, no idea how to proceed.
But I'd rather not implement something that doesn't work for keyboard/voice users.
I think it's best not to have it rather than a half-working feature :p

Jan mentioned adding a menu navigation feature. Where you click an ActionButton and it shift the whole menu on the left to make another set of entries open (instead of having a dropdown inside a dropdown).
Would this fix what you were trying to implement? :)

@korelstar
Copy link
Contributor Author

@skjnldsv I need to allow for input new values and comfortably select from existing values at the same time. If I understand Jan's idea correctly, that approach would allow for selecting existing options only. However, the first sub-action could be a plain input. This could work. But I'm unsure if this would be a good UX.

Copy link
Contributor

@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.

Looks good, but same as with the datepicker it’s strange that a hover triggers expanding the multiselect. I’d say only a focus event / clicking in the input should?

@nickvergessen
Copy link
Contributor

Looks good, but same as with the datepicker it’s strange that a hover triggers expanding the multiselect. I’d say only a focus event / clicking in the input should?

Was about to post the same thing.

@skjnldsv
Copy link
Contributor

skjnldsv commented Oct 7, 2020

I’d say only a focus event / clicking in the input should?

It's due to the way we manage focus feedback.
When you hover an entry it focuses it to keep the currently focused index of the menu list. Otherwise you could have one entry focused with keyboard and another hovered with your mouse

Capture d’écran_2020-10-07_08-23-20
Like here, first is active, second is hovered last is focused. Not great 😕
(And yes, this can happens naturally, I did not force any state nor opened the dev tools)

@skjnldsv
Copy link
Contributor

Ping @jancborchardt 😉
#1250 (comment)

@nickvergessen
Copy link
Contributor

This is currently blocking the announcement center rewrite to Vue as one of the last things:
nextcloud/announcementcenter#218

What's the status here? Can we merge it for now and fix the comment afterwards?

@jancborchardt
Copy link
Contributor

jancborchardt commented Dec 15, 2020

Hm, your call @skjnldsv on whether to do it in this PR or separately, but I still think that we should absolutely trigger neither datepicker nor multiselect on hover, we have to separate hover and focus there.

Even triggering on focus, would that be expected? Quick check on e.g. Airbnb doesn’t open their datepicker on focus, you have to focus and then press enter.

@skjnldsv
Copy link
Contributor

we have to separate hover and focus there.

I'm fine with that, but then you'll lose the persistance between the two. Hovering should kill the focus then.

@nickvergessen
Copy link
Contributor

@korelstar can you adjust this?

@korelstar
Copy link
Contributor Author

korelstar commented Jan 16, 2021

@nickvergessen Sorry I'm a little bit lost, here. I think the criticised behavior is out of scope from this PR. This PR introduces MultiSelect while adopting the behavior of DatePicker. From my understanding, DatePicker's behavior has to be fixed first (in another PR) and then I will adopt it in this PR (if the fix is not automatically applied). But please feel free to add some fixing commits to this PR.

@nickvergessen nickvergessen merged commit 2904ac4 into master Jan 18, 2021
@nickvergessen nickvergessen deleted the actioninput-multiselect branch January 18, 2021 08:54
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 feature: actions Related to the actions components feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActionInput: add Multisect
4 participants