Switch from Select to Combobox for Country and State Inputs #4369
Conversation
|
Size Change: +28.9 kB (+3%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Tests will need updating due to the different country select markup. |
There was a problem hiding this comment.
I really love how it works and looks. I think this is a big improvement to the user experience. 👏
I tested several flows and features:
- Desktop, mobile and screen reader.
- Several themes.
- Autofill.
- Validation.
It worked great in all of them!
The only thing that worries me a bit is the size increase in cart and checkout frontend scripts. But I think the increase is worth it considering how the combobox simplifies the filling experience.
To get the label working I needed to simulate onBlur.
If it's only for styling purposes, I wonder if we could use the :focus-within pseudoclass. Now that we don't need to support IE11, this is a new tool we can make use of.
| @@ -0,0 +1,156 @@ | |||
| .wc-block-components-form .wc-block-components-combobox, | |||
There was a problem hiding this comment.
Why is this selector required? If it is for specificity reasons, I would add a comment explaining that, and maybe using .wc-block-components-combobox.wc-block-components-combobox would be better so styles have the same specificity independently if it's inside a form component or not?
There was a problem hiding this comment.
I based this off of the select box control's CSS which has the same.
There was a problem hiding this comment.
Aaah, I see. I would make the change in both, but not blocking this PR, then.
mikejolley
force-pushed
the
add/combo-control
branch
from
db0ee7a to
c2dbda8
Compare
|
@Aljullu Feedback taken care of :) The focus-within change worked really well, thanks for that. |
There was a problem hiding this comment.
Thanks for updating this PR @mikejolley! I didn't repeat the tests in all devices, but I did it on desktop and it's working fine.
Something that I think would improve the user experience is auto-selecting the first option of the combobox by default, would that be possible?
In the example above, I'm writing estonia and pressing Enter, but the only suggestion available is not autoselected. Instead, I would need to press Key down + Enter or select it with the mouse. In trunk, however, writing a country name and pressing Enter works, so this is kind of a regression (not sure if blocking, though).
mikejolley
force-pushed
the
add/combo-control
branch
from
fd6a04f to
30411ee
Compare
|
@Aljullu That issue seems to be coming from the combo control: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/combobox-control/index.js#L112-L117 If you have a value already, then type, then hit enter, the above code triggers and returns your selection to the original value. You'll see that if you just type, or click out of the input, our custom logic triggers and keeps the new value selected. It doesn't look like we have control over the behaviour or enter. Any suggestions? |
There was a problem hiding this comment.
@Aljullu That issue seems to be coming from the combo control: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/combobox-control/index.js#L112-L117
Thanks for investigating! Tested the ComboboxControl from the GB storybook and this is how it works there too.
I still feel this behavior is a bit counter-intuitive. I would open an issue in Gutenberg and see if they agree with us and would accept PRs. If yes, maybe we can add a task to contribute a PR upstream to our next cooldown?
I left one comment below about an onChange() which runs on every render, but besides that, pre-approving.
| ...values, | ||
| [ field.key ]: '', | ||
| } ); | ||
| } |
There was a problem hiding this comment.
That means onChange() is called on each render, right? Could this be refactored so it's run inside a useEffect() when the country or state change?
|
Logged an issue here: WordPress/gutenberg#33920 |
|
@nerrad @senadir @Aljullu I've added something called Patch Package which allows us to patch one of our dependencies and have the patch applied after NPM install. I've used this to apply the fix from my upstream PR so something is in place until that is updated and we bump the version we're using. Are you happy with this solution? |
Nifty tool, first I learned of it! The only question I have is whether this particular usability improvement of combobox warrants the extra complexity to surface it now for all users, or if it's sufficient to just have the behaviour show up for sites running WordPress 5.9 (which is presumably when the fix will show up in WordPress core - sometime in December?) or if they have GB active? |
|
@nerrad By users do you mean us developers? This particular package is one of our direct imports ( |
|
No sorry, I mean users of whatever UI implements the component. |
|
We've chatted about my question in DM, I'm just leaving my feedback in here for reference. I was a bit concerned about the additional complexity for addressing a usability issue (a user typing something in the field powered by combobox and pressing enter and it not selecting the first matching item) given:
However, in conversing with Mike, it appears that the third point is more significant than I thought so I'm happy to defer to the choice to apply the patch until its no longer necessary. How will we make sure we remove the patch (and package) when it's no longer necessary? I haven't reviewed the actual code or tested this so I can't give a green check - I mostly just came into this PR to answer the question directly asked of me. But if it's good to go and tested, there's nothing blocking from me. |
There was a problem hiding this comment.
I tested it again on desktop and I love how it works now. It makes filling addresses much faster. 😍
Good job @mikejolley! 
How will we make sure we remove the patch (and package) when it's no longer necessary?
Should we open an issue (or add a @todo comment) so we don't forget about this?
mikejolley
force-pushed
the
add/combo-control
branch
from
e940e00 to
8530b00
Compare
|
Todo added! 👍🏻 |
mikejolley
added
type: enhancement
| onFilterValueChange={ ( filterValue: string ) => { | ||
| if ( filterValue.length ) { | ||
| // If we have a value and the combobox is not focussed, this could be from browser autofill. | ||
| const activeElement = isObject( controlRef.current ) |
There was a problem hiding this comment.
why was isObject used here? a dom element would never evaluate to true here.
This PR implements a new Combobox control which essentially allows text input to search a list of values. This is useful for country and state fields and is similar to what Woo Core has on the core checkout.
The control being used is the ComboBox Control from Gutenberg.
Fixes #1752
Looking for feedback and ideas to further polish. To get the label working I needed to simulate onBlur. Autofill was also challenging. I ended up making it search when the input changes which causes an immediate update so I am not 100% sure this is a good solution.
Accessibility
Tested Keyboard controls.
Screenshots
How to test the changes in this Pull Request:
I went through all the TwentyX themes and Storefront.
Changelog