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: #116 Add outputMetricUnits to config types and use in itinerary-… #538

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

amenk
Copy link
Contributor

@amenk amenk commented Feb 4, 2023

…body

@amenk amenk marked this pull request as draft February 4, 2023 20:07
@amenk
Copy link
Contributor Author

amenk commented Feb 4, 2023

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
Copy link
Contributor Author

amenk commented Feb 4, 2023

  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
Copy link
Contributor Author

amenk commented Feb 11, 2023

image

@amenk
Copy link
Contributor Author

amenk commented Feb 11, 2023

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
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
Copy link
Contributor Author

amenk commented Apr 12, 2023

Thanks, what do you mean exactly?

@miles-grant-ibigroup
Copy link
Collaborator

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

@amenk
Copy link
Contributor Author

amenk commented Apr 29, 2023

@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
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
Copy link
Contributor Author

amenk commented May 2, 2023

@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