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

fix: support distance display in metric units #736

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

doncicuto
Copy link

Hi there, the following PR deals with issue #116

I've added a useMetricUnits props to the affected components reported in the issue:

  • AccessLegSummary (package itinerary-body)
  • AccessLeg (package printable-itinerary)
  • TransitStopOption (package location-field)

This prop is optional with default value false which means that imperial units will be used. If useMetricUnits is set, metric units will be used instead.

I've also added some stories to test useMetricUnits in action:

image

As this is my first ever contribution to this project so I hope this first contribution is useful. Please don't hesitate to tell me what I should improve or change for my future contributions.

This an awesome project, thank's a lot for your effort and the quality of the code. It's great that you've published it as an open source project, hopefully I'll try to upload more contributions soon.

Cheers!

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Thanks so much for contributing! And thanks so much for your kind words about the project. Your prop-based solution is well-implemented, but I think we're hoping to use our localization library to implement this feature

@miles-grant-ibigroup miles-grant-ibigroup self-assigned this Apr 19, 2024
@doncicuto
Copy link
Author

Hi @miles-grant-ibigroup, sorry I didn't know about your localization library. Should I close this PR, do you want me to check that localization library? Let me know if I can be of any help. Thanks

@miles-grant-ibigroup
Copy link
Collaborator

Hi @miles-grant-ibigroup, sorry I didn't know about your localization library. Should I close this PR, do you want me to check that localization library? Let me know if I can be of any help. Thanks

You can keep this PR open! We use react-intl. This exposes an intl object that should contain the current locale. Figuring out whether to use metric or imperial based on this locale is the challenge. Perhaps a fixed list of imperial languages (en-US)?

I hope this is helpful. This is on our backlog to do ourselves, but we probably won't get to it soon.

@miles-grant-ibigroup miles-grant-ibigroup added the WIP Work in progress label Apr 25, 2024
@doncicuto
Copy link
Author

Hi there. It seems that only three countries still use the imperial units so I can hardcode those countries and use react-intl. If you are not in a hurry I'd like to work on this issue.

@miles-grant-ibigroup
Copy link
Collaborator

Hi there. It seems that only three countries still use the imperial units so I can hardcode those countries and use react-intl. If you are not in a hurry I'd like to work on this issue.

That sounds great! Please do it's much appreciated. I'll be here to review when you're ready :)

@doncicuto
Copy link
Author

Great @miles-grant-ibigroup. How about using this?

  // Reference: https://en.wikipedia.org/wiki/Mile
  // While most countries abandoned the mile when switching to the metric system,
  // the international mile continues to be used in some countries, such as Liberia,
  // Myanmar, the United Kingdom and the United States. It is also used in a number
  // of territories with less than a million inhabitants, most of which are UK or US territories,
  // or have close historical ties with the UK or US: American Samoa, Bahamas, Belize,
  // British Virgin Islands, Cayman Islands, Dominica, Falkland Islands, Grenada,Guam,
  // The N. Mariana Islands, Samoa, St. Lucia, St. Vincent & The Grenadines, St. Helena,
  // St. Kitts & Nevis, the Turks & Caicos Islands, and the US Virgin Islands.
  // The mile is even encountered in Canada, though this is predominantly in rail transport
  // and horse racing, as the roadways have been metricated since 1977.
  const useMetricUnits = [
    "en-US",
    "en-GB",
    "en-LR",
    "en-AS",
    "sm-AS",
    "en-BS",
    "en-BZ",
    "es-BZ",
    "en-VG",
    "en-KY",
    "en-DM",
    "en-GD",
    "en-GU",
    "en-MP",
    "en-WS",
    "sm-WS",
    "en-LC",
    "en-VC",
    "en-SH",
    "en-KN",
    "en-TC",
    "en-VI",
    "my-MM"
  ].includes(intl.locale);

@doncicuto
Copy link
Author

Hi @miles-grant-ibigroup I've modified the PR adding a function to check if according to react-intl locale we have to use metric or imperial units. Now fewer files are modified.

Here are two screenshots showing the changes of locale in action and how the units are changed.

Default locale (en-US):
Captura desde 2024-04-26 11-00-58

Using Spanish:
Captura desde 2024-04-26 11-01-07

Also, hope you don't mind, as I'm a Spaniard I've changed some entries for the itinerary-body es.yaml

Please let me know if you want me to change anything

Cheers!

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

This is a fantastic contribution thank you so much! I really appreciate your translation updates as well.

I have one review comment that would clean up the code. Once that's done I'll assign to a second reviewer! We do 2 reviews per PR -- once the second reviewer approves we will merge!

Comment on lines +112 to +113
coreUtils.metricUnits.areMetricUnitsUsed(intl.locale),
intl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to refactor humanizeDistanceString to use the intl object it gets anyway to avoid having to import coreUtils everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say don't remove the manual override (for backwards compatibility) but instead use it as an override -- a way to force metric even if the locale isn't metric

@miles-grant-ibigroup
Copy link
Collaborator

miles-grant-ibigroup commented Apr 26, 2024

As for the failing unit tests running yarn unit -u will update the snapshots!

@doncicuto
Copy link
Author

Thanks @miles-grant-ibigroup I'll prepare the changes

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

Successfully merging this pull request may close these issues.

2 participants