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

Create CountryInput component #1574

Closed
2 of 3 tasks
Aljullu opened this issue Jan 15, 2020 · 21 comments
Closed
2 of 3 tasks

Create CountryInput component #1574

Aljullu opened this issue Jan 15, 2020 · 21 comments
Assignees
Labels
type: enhancement The issue is a request for an enhancement.
Milestone

Comments

@Aljullu
Copy link
Contributor

Aljullu commented Jan 15, 2020

Part of #1348.

We need an input field that allows introducing a country.

imatge

This is how it currently looks in the Cart page:
imatge

Breakdown

@Aljullu Aljullu self-assigned this Jan 15, 2020
@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label type: feature request to this issue, with a confidence of 0.99. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@Aljullu
Copy link
Contributor Author

Aljullu commented Jan 15, 2020

I think the ComboboxControl that might be included in @wordpress/components (WordPress/gutenberg#19657) would be a good fit to use here.

So I would create the CountryInput using the CustomSelectControl component for now and switch to the ComboboxControl once it's released.

@senadir
Copy link
Member

senadir commented Jan 15, 2020

What data source are you intending to use?
Some considerations are that Woo Core limits can countries you can ship to, this can be:

  • One country, in which we use a span instead of a select.
  • A select set of countries, in which we use those countries.
  • No option specified, in which we use all countries.

The best place to consume this data is to get it from Woo (with translation and all), so new API endpoints might be required.

@Aljullu
Copy link
Contributor Author

Aljullu commented Jan 15, 2020

My idea was to use the values returned by WC()->countries->get_shipping_countries() and WC()->countries->get_allowed_countries() depending on if it's used in the shipping or the billing.

I was planning to set those values in the settings and directly read them from there instead of creating a new API endpoint, which seemed to be overwhelming. But I don't have a strong opinion on that, since we will need an API endpoint anyway for the CountryInput.

@senadir
Copy link
Member

senadir commented Jan 15, 2020

I was planning to set those values in the settings and directly read them from there instead of creating a new API endpoint, which seemed to be overwhelming. But I don't have a strong opinion on that, since we will need an API endpoint anyway for the CountryInput.

I also don’t have strong opinions about this as well, but this data might be big so I'm not sure if we should include it with every wcSettings print, I would love to hear from @mikejolley and @nerrad

@nerrad
Copy link
Contributor

nerrad commented Jan 15, 2020

We can conditionally populate the wcSettings global using the AssetDataRegistry (so you could only add countries for blocks that utilize the country component).

Long term it'd be nice to use a new endpoint devoted to this (is there already one?).

Before doing anything on this though, have you investigated what wc-admin is doing for country/state selectors? I don't think there's anything publicly exposed yet but I'm pretty sure there's something in their client code. It'd be useful I think to evaluate what wcAdmin is doing and see if that's something we can collaborate on so we have something usable by both.

@senadir
Copy link
Member

senadir commented Jan 15, 2020

Long term it'd be nice to use a new endpoint devoted to this (is there already one?).

seems it does exactly what we need.

@Aljullu
Copy link
Contributor Author

Aljullu commented Jan 16, 2020

Thanks for linking that endpoint @nerrad. If I'm not wrong, that endpoint lists all countries, which is not exactly what we need here, we only want countries where the store ships or sells. AKA instead of get_countries() we want get_shipping_countries() and get_allowed_countries() (see WC_Countries API docs).

We could update that endpoint to receive a new parameter to know which method to use. Does that make sense? I'm not used to work with the woocommerce-rest-api repo so I'm not sure what are the procedures or if that's doable at all.

In any case, I will create a PR with the <CountryInput> UI for now, so we can start getting this reviewed.

Before doing anything on this though, have you investigated what wc-admin is doing for country/state selectors? I don't think there's anything publicly exposed yet but I'm pretty sure there's something in their client code. It'd be useful I think to evaluate what wcAdmin is doing and see if that's something we can collaborate on so we have something usable by both.

Yes, I did take a look at their implementation of the country autocompleter and it's also listing all countries. For WooCommerce Admin it makes sense: you might have stopped selling to a country, but you might still want to filter data from previous customers from that country. But in our case we should only list countries were the store sells/ships.

@nerrad nerrad added type: enhancement The issue is a request for an enhancement. and removed type: feature request labels Jan 24, 2020
@nerrad nerrad added this to the Future Release milestone Jan 27, 2020
@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 11, 2020

@nerrad @mikejolley Would make it sense updating the countries endpoint of woocommerce-rest-api so it accepts a parameter to know whether it must return all countries (default), shipping countries or allowed countries?

@nerrad
Copy link
Contributor

nerrad commented Feb 11, 2020

Would make it sense updating the countries endpoint of woocommerce-rest-api so it accepts a parameter to know whether it must return all countries (default), shipping countries or allowed countries?

Hmm, it sounds like something that should be a resource on the /shipping/ endpoint? So something like /shipping/countries?

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 11, 2020

We would need an endpoint to return the shipping countries (get_shipping_countries()) and countries where the shop sells (get_allowed_countries()). Do you think the second use case makes sense under the /shipping/ prefix?

@nerrad
Copy link
Contributor

nerrad commented Feb 11, 2020

I'm not sure I understand fully what the difference is between get_allowed_countries and get_shipping_countries and all the countries returned by the /countries endpoint.

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 11, 2020

The endpoint only supports the first use-case. But for the Cart and Checkout blocks we need the second and third. So we either extend that endpoint or we create a new one.

In the UI, you can find these settings in WooCommerce > Settings:
imatge

Hope that helps. 🙂

@nerrad
Copy link
Contributor

nerrad commented Feb 11, 2020

Thanks Albert, I probably could have taken the time to figure that out myself but I appreciate the explanation ❤️ .

I'm happy to defer to @mikejolley 's thoughts on this. From my perspective:

  • if you think there will be more contexts where the different country variations would be needed (other than just shipping) then enhancing the existing endpoint probably is desirable.
  • if you think the additional variations will be mostly just needed in the context of shipping, then it might be good to just enhance the shipping route (/shipping/allowed-countries and shipping/countries)

@mikejolley
Copy link
Member

@Aljullu The countries endpoint being used now is not from the store API correct? Thus requires auth. Is that suitable?

Since the list of countries is static, would it make sense to include this via wcSettings?

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 12, 2020

@mikejolley

@Aljullu The countries endpoint being used now is not from the store API correct? Thus requires auth. Is that suitable?

Oh, good point. We would need all users to have access to it.

Since the list of countries is static, would it make sense to include this via wcSettings?

That was my initial thought as well, and how it's currently implemented. But that solution would not escale well for counties/states/provinces because the list will be much larger. Given that for counties/states/provinces we will need an endpoint, we thought it made sense loading countries from and endpoint as well. Does that make sense?

@mikejolley
Copy link
Member

mikejolley commented Feb 12, 2020

@Aljullu Hmm what is the concern with the large data set? Using an API will cause noticeable delay when changing countries since you'll need to wait for states to come back. Is that also a concern?

States might be needed conditionally, but countries won't. You always need the full list.

@senadir
Copy link
Member

senadir commented Feb 12, 2020

I think it would make more sense to differ states and counties until everything is loaded and the user started filling his form, however, some browsers do offer full autocomplete to all fields, so I'm not sure how will that play with a field that didn’t load yet, could we possibly use the MaxMind Geolocation to decide what we want to serve initially?

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 12, 2020

@mikejolley so if I understand you correctly, you propose to have both (countries & counties) in wcSettings? I was looking at how WooCommerce currently works and it seems to hardcode all values in the page markup (it can be seen with console.log( wc_country_select_params )), so you might be right we are over-complicating things trying to create an endpoint for this.

some browsers do offer full autocomplete to all fields, so I'm not sure how will that play with a field that didn’t load yet

@senadir yeah... I think we would have this issue anyway no matter if we have the values pre-loaded or we load them through an endpoint. Because we need one of the selects to change based on the value of another one so both can't be filled in parallel. But it might be worth doing some testing once we have the forms set up.

@nerrad
Copy link
Contributor

nerrad commented Feb 12, 2020

Putting countries and counties (states/provinces) in wcSettings sounds like a good initial step to get the ball rolling, however we'll want to make sure they are only available on routes needing it (i.e. use the AssetDataRegistry api to queue up the countries data in the related php block registration needing it).

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 17, 2020

I created a follow-up (#1752) for the last task of this issue, so closing this one.

@Aljullu Aljullu closed this as completed Feb 17, 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
type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

No branches or pull requests

4 participants