-
Notifications
You must be signed in to change notification settings - Fork 38
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
refactor(Combobox): test implementing u-tags / u-datalist #2780
base: next
Are you sure you want to change the base?
Conversation
|
Preview deployments for this pull request: Storybook - |
Coverage Report
File Coverage
|
I'll add some comments here, before trying this in another PR:
A lot of this seems to be out of our hands, and is the reason I am a bit worried about going for a solution that is not made by us - we loose too much control over what happens. I have tried this with the pure component from u-elements as well, and some of the behaviour seems to come from there. I see this is also not using the popover API, like we talked about. Is this not possible, or did you just not implement it? I don't see it as a sustainable solution to update u-elements to get behavior we want, but I'll branch out from this and try some things myself. |
Thanks for thorough testing @Barsnes 🙌 For instance:
What causes a select can be controlled, and what shows is up to us since we can listen both to clicks on options, as well as typing, and overwrite default behavior.
This is intentional and how both ARIA combobox and native datalist :)
This is also intentional - after user testing, we saw clicking the chip itself caused confusion or margin for error among some users. Therefore, we moved the click-functionality to the ×-only, so that click on the chip it self sets focus and thus makes it also possible to delete by keyboard. Making hover and focus states for the ×-area can help visualize this concept even better :)
This might actually be a bug - we'll look into it 🙌 🫶 Regarding Popover API: we are actually working on implementing so that datalist uses this when supported by the browser. |
I wish we documented why we removed looping, but our current implementation used to have it, but was removed: #1340 Some of the answers here is also why we decided to make this ourselves first - we want to implement decisions we make, and not rely on a third party to fix it on their end. I'll try to do some of the things I mentioned above, since you feel they are doable - but it seems maybe like a lot of hotwiring 🤔 Otherwise great POC, lots of good things here. |
If the project is very worried; feel free to copy the source code out of u-elements - most important is to get the user benefits. If a bug is found; you can contribute just like in any other open source project, and fixing the bug will gain more people than DigiDir Designsystem. Your worries are in many ways the same as for the users we ask to use Deisgnsystemet; some control is always lost when depending on something, while gaining something else. Regarding hotwiring - I can add an example if you want. Default behavior of u-datalist will always comply with the native datalist and combobox specification, so yes; moving away from the standardard will require some more code on our end, but still much less wiring than creating everything from scratch. Regarding looping; it would be very interesting to hear why this was removed also for us in the u-elements project <3 |
Since we are using the actual
Should we also do this for our removable
Is this something we from the team can help do? Contribute back and get to know some web components :) |
I would not worry about using this particular library as we have the primary developer on our team, so its not the same as using for example I am also sure @eirikbacker is willing to help us if we find any bugs, and beside we should contribute back when possible, not to mention get some experience working with web-components! 😊 |
#2610
multiple
andonChange
,Combobox.Clear
andCombobox.Empty
filter
or externalCombobox.Loading
, but this will be quite straight forawrduseCombobox
hook first, but caused a lot of wiring for the consumer, and potential props-conflics, so found it easier to resolve and hide this complexity by usingcontext
.