Skip to content

Add More Flow Units #361

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

Merged
merged 2 commits into from
Jan 14, 2018
Merged

Add More Flow Units #361

merged 2 commits into from
Jan 14, 2018

Conversation

gojanpaolo
Copy link
Contributor

Fixes #360

@gojanpaolo gojanpaolo mentioned this pull request Jan 10, 2018
3 tasks
@eriove eriove self-requested a review January 11, 2018 19:24
Copy link
Contributor

@eriove eriove left a comment

Choose a reason for hiding this comment

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

I've cross checked all the conversion factors and have some minor comments on the abbreviations.

"Localization": [
{
"Culture": "en-US",
"Abbreviations": [ "galUS/s" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

In volume we've written gal (U.S.). Then gal (U.S.)/s would be consistent here, but that seems awkward. @angularsen what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually display US Gallons as gal (US).
Takes less space and seems to be clear enough.
Also less awkward when used in Flow gal (US)/s

We also display US Survey Feet as ft (US) ... ft (US)/s

Copy link
Owner

Choose a reason for hiding this comment

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

It is awkward, but at least it is consistent. I think it's fine.

"Localization": [
{
"Culture": "en-US",
"Abbreviations": [ "galUS/h" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment regarding gal (U.S.) here

"Localization": [
{
"Culture": "en-US",
"Abbreviations": [ "cf/hr" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

The official abbreviation of cubic foot is ft³ and cf isn't listed on Wikipedia. I would suggest to change this to ft³/h and perhaps add this as an alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. :)

"Localization": [
{
"Culture": "en-US",
"Abbreviations": [ "ft³/h", "cf/hr" ]
Copy link
Owner

Choose a reason for hiding this comment

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

Is cf/hr used? Having it here means you expect to parse it.
If we keep this, should the same be added for cubic yards per hour below?

Copy link
Contributor Author

@gojanpaolo gojanpaolo Jan 13, 2018

Choose a reason for hiding this comment

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

cf/hr was already there before this PR so I didn't remove it. I suggest we put the task of removing it in the wishlist for breaking changes.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah I missed that. Good point, if you could add it to the wishlist that would be good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! :)

@gojanpaolo gojanpaolo mentioned this pull request Jan 14, 2018
13 tasks
@angularsen
Copy link
Owner

Alrighty, this should be good to go then

@angularsen angularsen merged commit 40cd8ad into angularsen:master Jan 14, 2018
@angularsen
Copy link
Owner

Nuget 3.87 on the way out

@gojanpaolo
Copy link
Contributor Author

Thank you! 💯

@gojanpaolo gojanpaolo deleted the AddMoreFlowUnits branch January 14, 2018 04:08
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