Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Create CountyInput component #1727

Merged
merged 8 commits into from
Feb 14, 2020
Merged

Create CountyInput component #1727

merged 8 commits into from
Feb 14, 2020

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Feb 13, 2020

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

Peek 2020-02-13 12-09

How to test the changes in this Pull Request:

  1. Create a post with a Checkout block and modify this line so input fields appear:
-{ shippingMethods.length > 0 && (
+{ true && (
  1. Select a country with counties (United States or Spain for example), and verify the county input appears.
  2. Try selecting a country with no counties (Andorra, for example), and verify the county input is hidden.

@Aljullu Aljullu added status: needs review focus: components Work that introduces new or updates existing components. labels Feb 13, 2020
@Aljullu Aljullu added this to the Future Release milestone Feb 13, 2020
@Aljullu Aljullu requested a review from a team as a code owner February 13, 2020 11:22
@Aljullu Aljullu self-assigned this Feb 13, 2020
@Aljullu Aljullu requested review from haszari and removed request for a team February 13, 2020 11:22
@Aljullu Aljullu mentioned this pull request Feb 13, 2020
4 tasks
@Aljullu Aljullu changed the base branch from update/country-settings-available-when-needed to master February 13, 2020 12:54
Copy link
Member

@mikejolley mikejolley left a 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 :)

  1. 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.
  2. 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).
  3. 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.
  4. No state input when selecting united kingdom?
  5. Does the little down arrow icon on the right need to toggle once open?
  6. 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

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 13, 2020

Thanks for the review and the great feedback, @mikejolley!

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

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?

  1. 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).

Right, this and some other issues with keyboard navigation should be fixed with 4ad3084.

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

Good catch. I will try to upstream this to the <CustomSelectControl> component. But I think it should use the default cursor instead of the pointer (aka arrow instead of hand). At least this is how it looks for me in native selects.

  1. No state input when selecting united kingdom?

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.

  1. Does the little down arrow icon on the right need to toggle once open?

I don't think so. Native selects don't toggle the arrow (at least on Firefox & Chrome under Linux).

  1. Why are counties all caps in this input? Other inputs do not change text like this.

What do you mean all caps? This is how it looks for me:

imatge

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

Agree. We have an open issue for that: #1562.

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 13, 2020

I will try to upstream this

I created a PR in Gutenberg: WordPress/gutenberg#20217.

@mikejolley
Copy link
Member

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?

Ok, I was going by the core country dropdown. That keeps the parent in focus on click.

But I think it should use the default cursor instead of the pointer (aka arrow instead of hand). At least this is how it looks for me in native selects.

Fine with me. Again I was comparing to the core select, but if it matches native thats fine.

I don't think so. Native selects don't toggle the arrow (at least on Firefox & Chrome under Linux).

Core again :) We can leave it it doesn't make much difference.

What do you mean all caps? This is how it looks for me:

Possibly theme issue. It's caps in twentysixteen

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 13, 2020

Oh, I see. The Core select works a bit differently. In this case, I would prefer to stick to the way the Gutenberg CustomSelectControl component works. It's more similar to native selects and it's going to be easier to mantain.

Possibly theme issue. It's caps in twentysixteen

Right! I added reset styles in 627f49e.

This should be ready for another look.

Copy link
Member

@mikejolley mikejolley left a 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.

@Aljullu Aljullu merged commit 75c6328 into master Feb 14, 2020
@Aljullu Aljullu deleted the add/county-input branch February 14, 2020 12:30
@Aljullu Aljullu mentioned this pull request Feb 21, 2020
@nerrad nerrad modified the milestones: Future Release, 2.6.0 Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: components Work that introduces new or updates existing components.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants