Skip to content
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

default_region: add region_a to label where available #41

Closed
wants to merge 1 commit into from

Conversation

missinglink
Copy link
Member

Using the default schema it's often difficult to distinguish localities with the same name in the same country, this makes selecting the correct result impossible:

Screenshot 2021-02-04 at 23 11 07

In other cases it would be nice to see some more information about which general area of the country a place is in:

Screenshot 2021-02-04 at 23 15 20

This PR adds the region_a value where available (except for the region layer itself, which would be redundant).

It's been this way for a very long time but I think this is an improvement, ideally I would like to see four things in the label:

{name}, {locality}, {region}, {country}

I looked at using the region label instead of region_a but it can generate some labels which sound repetitive like "Wellington, Wellington City" and "Chiang Mai, Chiang Mai".

@orangejulius
Copy link
Member

Yeah, I believe this is the correct thing to do.

I took a look at this yesterday only for Brazil, and I think using the existing getRegionalValue method in all labels would work well.

That method has handling for region placetypes, and preference to use region_a but fallback to region (the name).

Copy link
Member

@Joxit Joxit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Joxit
Copy link
Member

Joxit commented Feb 4, 2021

region_a is a good agreement, this should be small and will create less noise than the full region name.

And this will not change the FRA overseas where the region replaces the country behavior so 👍

@missinglink
Copy link
Member Author

@Joxit I think this change will leave FRA as the only country with only two parts in the schema.

Is France good as-is or should we do something similar there?

@Joxit
Copy link
Member

Joxit commented Feb 4, 2021

IFAIK, we don't use region abbreviations (I didn't know there were 😅)
We use the full region name or the postalcode. For example in our demo website, when you are looking for the city Bagneux (7 Bagneux in France 😱), the label looks like {locality} - {country}, {region} only when there are duplicates.

image

But when you are looking for a city without duplicates, the label looks like {locality} - {country}, for example Paris - France:
image

So I think France is good as-is 😅

@missinglink
Copy link
Member Author

Hmm... think you're generating your own labels?
Both the default labels and default results are quite different from what's shown in your screenshots 🤷‍♂️

Screenshot 2021-02-05 at 13 08 33

Screenshot 2021-02-05 at 13 10 08

In both cases I think the results from Jawg are better, any idea what config you're using for that?

@Joxit
Copy link
Member

Joxit commented Feb 5, 2021

Good morning 😴

This is a custom integration for our demo, so the job is not done by the API (for a better DX, this should be done by the API..)

When we receive all records, we checks duplicates and we update them.

If we want this in Pelias, we will need a Middleware after the label which will check duplicated labels and rename them 🤔

So this will need a work in both the API and this module where the region part can be optional for exemple?

orangejulius added a commit to pelias/api that referenced this pull request Feb 5, 2021
orangejulius added a commit to pelias/api that referenced this pull request Feb 5, 2021
@orangejulius
Copy link
Member

Okay, I ran the acceptance tests with this change to get an idea of what a bunch of labels will look like.

I think it adds a bit too many abbreviations. In particular, if the label already includes a city, and there's also a region of the same name, I don't think we should show the abbreviation. For example:
image

We might not be able to easily detect this case without adding a bunch more logic. Looks like @Joxit might be working on exactly that in bbf316a :)

@Joxit
Copy link
Member

Joxit commented Feb 5, 2021

Berlin is a good edge case for my PR... 😕 Berlin is both a city AND a region.... Just like Paris... So when we add the region part, this will add nothing more...

Maybe withOptional is too simple for what we want to do ? We should start with something complicated/more specific on regional part ? To be, or not to be, that is the question 🤷

@orangejulius
Copy link
Member

orangejulius commented Feb 5, 2021

Yeah, I think there are several aspects to a good solution:

  • always displaying a regional value (whether the full name or abbreviation) is probably not a good default for the whole world. It seems that only a few countries, like the US, always want the region in the label
  • We want to show the region (either the name or abbreviation, if available) when it disambiguates multiple otherwise identical cities
  • However, as an exception to the above, we don't want to show the region name or abbreviation when the region name matches the city name (this is the case in Berlin, Sao Paulo, Paris, Tokyo, and others)

@orangejulius
Copy link
Member

Replaced with #43 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants