-
Notifications
You must be signed in to change notification settings - Fork 221
Conversation
05dbe32
to
884c8ee
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.
I tested this and it worked as described. Code looks good.
I have some feedback on the control itself; maybe for follow up? Or in this PR. Your choice :)
- Most of the fields on checkout have a focus state; this dropdown does not. Click the input and you'll see the border color does not change like other fields. It does have focus style when tabbing to the input however.
- After selecting a value, if you open the dropdown again it starts at the top item, rather than showing your selection. Compare to a regular select input to see what I mean here. This also means if you use keyboard to navigate items you have to start from the first item (aland islands).
- There is a mix of regular cursor/text cursor when hovering items in the list. This should use cursor:pointer (the hand) for all items in the list.
- No state input when selecting united kingdom?
- Does the little down arrow icon on the right need to toggle once open?
- Why are counties all caps in this input? Other inputs do not change text like this.
The only other issue I noted, not specific to this field, was that browser autocomplete did not trigger. This may be because the fields have no names -- we should resolve this. https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete
Thanks for the review and the great feedback, @mikejolley!
I'm not sure to understand you. When tabbing I see the normal focus styles, when clicking on it the styles appear for a while but disappear when the dropdown is shown, since it catches the focus. Moving through the options with the keyboard shows each one with a gray background. What would you can change from this behavior?
Right, this and some other issues with keyboard navigation should be fixed with 4ad3084.
Good catch. I will try to upstream this to the
I thought all countries had specific options for the county, but it looks like some allow free entering the name. I didn't account for that. Should be fixed in 42f7725.
I don't think so. Native selects don't toggle the arrow (at least on Firefox & Chrome under Linux).
What do you mean all caps? This is how it looks for me:
Agree. We have an open issue for that: #1562. |
I created a PR in Gutenberg: WordPress/gutenberg#20217. |
Ok, I was going by the core country dropdown. That keeps the parent in focus on click.
Fine with me. Again I was comparing to the core select, but if it matches native thats fine.
Core again :) We can leave it it doesn't make much difference.
Possibly theme issue. It's caps in twentysixteen |
Oh, I see. The Core select works a bit differently. In this case, I would prefer to stick to the way the Gutenberg
Right! I added reset styles in 627f49e. This should be ready for another look. |
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.
There is a merge conflict @Aljullu then this can merge.
627f49e
to
f39f0a3
Compare
9d3c296
to
806d960
Compare
Part of #1575.
Built on top of #1726.
In a follow-up I will make the word County change depending on the country (sometimes it must be State, Province, District, etc.).
Screenshots
How to test the changes in this Pull Request: