-
Notifications
You must be signed in to change notification settings - Fork 92
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
Conversation
822dd48
to
1515b88
Compare
There was a problem hiding this 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 :)
Other than this! Great addition!! 👍 |
Ah, it doesn't seems that you can :/ |
Added the |
@@ -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 }" |
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test it? For me it works (Firefox). I think it works due to https://github.com/shentao/vue-multiselect/blob/d8d29e7714449f4fca4bfe10d3bd21e72c46c9b7/src/Multiselect.vue#L5
There was a problem hiding this comment.
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 🤷
@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? |
#1250 (comment) 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). |
@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. |
There was a problem hiding this 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?
9465d7c
to
32494ed
Compare
Was about to post the same thing. |
Ping @jancborchardt 😉 |
This is currently blocking the announcement center rewrite to Vue as one of the last things: What's the status here? Can we merge it for now and fix the comment afterwards? |
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. |
I'm fine with that, but then you'll lose the persistance between the two. Hovering should kill the focus then. |
@korelstar can you adjust this? |
@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. |
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