Skip to content

Conversation

@ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Dec 20, 2023

Problems

Problem 1.

In the timezone selector the countries are only displayed if they are part of the name of the timezone (eg. America/Argentina/Buenos_Aires). In most cases, the name of the timezone includes only the global region and a concrete city (eg. Europe/Madrid). That makes hard to find the right timezone since the most intuitive mechanism (typing the name of the user country in the search box) is usually useless.

spain-old

Problem 2.

There are some duplicated or controversial timezones that are not associated to any country. For more details this gist.

buenos_aires-old

Solution

Add the "most representative country" to each timezone and display that in the UI. That information is read from the file /usr/share/zoneinfo/zone.tab.

That makes it easier to find timezones.

spain-new

Timezones without a country (except those that really make sense) are filtered out. That removes controversy and duplicates.

buenos_aires-new

Works even for the two timezones that have a different identifier at langtable and at zone.tab. Again, see the previously mentioned gist.

rangoon-new

Testing

  • Added new unit tests
  • Tested manually (as shown in the screenshots)

@ancorgs ancorgs force-pushed the timezone_small_fixes branch 2 times, most recently from d8f89bb to be47222 Compare December 21, 2023 09:32
@coveralls
Copy link

coveralls commented Dec 21, 2023

Coverage Status

coverage: 74.758% (-0.002%) from 74.76%
when pulling a1d0c0d on timezone_small_fixes
into f55671d on master.

@ancorgs ancorgs force-pushed the timezone_small_fixes branch from be47222 to 0da4a48 Compare December 21, 2023 09:49
@ancorgs ancorgs marked this pull request as ready for review December 21, 2023 10:00
@ancorgs ancorgs force-pushed the timezone_small_fixes branch from 0da4a48 to 17d00fc Compare December 21, 2023 10:26
@ancorgs ancorgs force-pushed the timezone_small_fixes branch from 17d00fc to ad42b0f Compare December 21, 2023 10:29
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

The code LGTM. But, please, update the changes files.

@joseivanlopez
Copy link
Contributor

joseivanlopez commented Dec 21, 2023

Maybe it is a matter of taste, but using one more column makes the selector to look a bit crowded. What about showing all parts in the first column and country in the second?

Africa - Ceuta                 Spain              10:10
Atlantic Ocean - Canary        Spain              09:10  

Timezones without country would have an empty second column, but that cases are only some exceptions.

@ancorgs
Copy link
Contributor Author

ancorgs commented Dec 21, 2023

What about showing all parts in the first column and country in the second?

I have no strong preference. So let's see.

This is how it looks now in this PR (keeping what existed and just adding a column for the country):

four-column-a
four-column-b
four-column-c

And this is how it would look what you suggest (merge the parts in to the first column and use the second for the country):

three-columns-a
three-columns-b
three-columns-c

@dgdavid, @imobachgs, @joseivanlopez What do you prefer?

@imobachgs
Copy link
Contributor

@dgdavid, @imobachgs, @joseivanlopez What do you prefer?

I would choose the second option as it is more clear to me: the localized name of the timezone and the country. If I see more columns, I need to somehow think what they are all about.

Having said that, I do not see the reason to use a hyphen to separate the components, but let's keep that for a different discussion.

@ancorgs
Copy link
Contributor Author

ancorgs commented Dec 21, 2023

If I see more columns, I need to somehow think what they are all about.

Good argument. Let's go for the second option then. I just added a commit to change that (although I don't think I will update the screenshots in the description).

@ancorgs ancorgs merged commit 20c3aa3 into master Dec 21, 2023
@ancorgs ancorgs deleted the timezone_small_fixes branch December 21, 2023 13:04
@imobachgs imobachgs mentioned this pull request Dec 21, 2023
imobachgs added a commit that referenced this pull request Dec 21, 2023
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.

5 participants