Skip to content

fix: #116 Add outputMetricUnits to config types and use in itinerary-… - #538

Draft
amenk wants to merge 3 commits into
opentripplanner:masterfrom
AddisMap:116-use-metric-config
Draft

amenk wants to merge 3 commits into
opentripplanner:masterfrom
AddisMap:116-use-metric-config

Conversation

@amenk

@amenk amenk commented Feb 4, 2023

Copy link
Copy Markdown
Contributor

…body

@amenk
amenk marked this pull request as draft February 4, 2023 20:07
@amenk

amenk commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author

TODO:

  1. Add to

packages/location-field/src/options.tsx:90

{humanizeDistanceString(stop.dist, config.outputMetricUnits,true)}

But we have to pass through the config somehow.

  1. Also in the printable printable-itinerary still imperial units are used, but I couldn't figure out where this is rendered.

@amenk

amenk commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author
  1. using react dev tools I confirmed that the config is correct in the print-component - I actually think this is related to my workflow (trip form: add callback for detecting when all submodes are disabled #773), because printable-itinerary depends on itinerary-body and probably is using the old version. So not a real issue.

@amenk

amenk commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

image

@amenk

amenk commented Feb 11, 2023

Copy link
Copy Markdown
Contributor Author

Not sure if c389bed is the right approach (to pass the outputMetricUnits into the location field component) of if we should access some global configuration from there.

@miles-grant-ibigroup

Copy link
Copy Markdown
Collaborator

Sorry for the long delay on this, but have finally had the time to take a look at this. I don't mind this approach, however is it not possible to use react-intl to handle this?

@amenk

amenk commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

Thanks, what do you mean exactly?

@miles-grant-ibigroup

Copy link
Copy Markdown
Collaborator

Is https://formatjs.io/docs/react-intl/api#formatnumber not appropriate here?

@amenk

amenk commented Apr 29, 2023

Copy link
Copy Markdown
Contributor Author

@miles-grant-ibigroup I don't know. As you can see from the PR, there is already a humanizeDistanceString function in the code base which can be used.

It's more about passing the setting, whether to use imperial or metric all the way down to the components.

@miles-grant-ibigroup

Copy link
Copy Markdown
Collaborator

Thanks for clarifying, I've discussed it with others and I think your approach is the best way to implement this feature. Once the PR is ready the approach has my approval!

@amenk

amenk commented May 2, 2023

Copy link
Copy Markdown
Contributor Author

@miles-grant-ibigroup cool :-) Actually I have technical problems to complete the PR ... still this yarn linking stuff :-(
In case you want to pick up...

There are really more countries (luckily IMO) using the metric system than the imperial one ;-)

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.

2 participants