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

Show gnomad in number #2987

Merged
merged 4 commits into from
Jan 22, 2020
Merged

Conversation

leexgh
Copy link
Member

@leexgh leexgh commented Jan 15, 2020

show gnomad in number with 2 non-zero digits after decimal point

left align with different length of number
image

@leexgh leexgh requested a review from onursumer January 15, 2020 17:40
@@ -145,7 +145,8 @@ export default class GnomadFrequency extends React.Component<GnomadFrequencyProp
if (result["Total"].alleleFrequency === 0) {
display = <span>0</span>;
} else {
display = <span>{parseFloat(result["Total"].alleleFrequency.toString()).toExponential(1)}</span>;
// show frequency as number with 4 significant digits
display = <span>{significantDigits(result['Total'].alleleFrequency, 4)}</span>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we have a library called Numeral.js (google it for docs) which should do this. lets use it instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alisman thanks! Basically what I'm looking for is to "round" number to 4 significant digits. I take a look at the Numeral.js doc but I'm not sure if it has the function I need, but I found another way to do it:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat
do you think it's ok to use?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, i see you are right numeral.js doesn't seem to have min/max significant digits. I put another comment in. As a general rule, try to avoid writing library code unless necessary and discuss with team if you think you do need to do so.

@@ -146,7 +145,11 @@ export default class GnomadFrequency extends React.Component<GnomadFrequencyProp
display = <span>0</span>;
} else {
// show frequency as number with 4 significant digits
display = <span>{significantDigits(result['Total'].alleleFrequency, 4)}</span>;
var significantDigitsFormatter = new Intl.NumberFormat("en", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think this is fine. Support looks good. However, I would subsistute the following https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString

It has the same options and doesn't confuse so much with "Intl"

Also, it seems that passing the locale ("en") is optional. If so, we should just not pass anything as we are not localized and we intend to use same formatting for any locale.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alisman thanks, one minor thing is that it looks like I have to pass a locale anyway, so I pass undefined there.

@@ -1,6 +1,6 @@
{
"name": "react-mutation-mapper",
"version": "0.4.2",
"version": "0.4.3",
Copy link
Member

Choose a reason for hiding this comment

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

we should also increment the version for cbioportal-frontend package.json

@alisman
Copy link
Collaborator

alisman commented Jan 17, 2020

awaiting @jjgao approval of the number column treatment

@jjgao
Copy link
Member

jjgao commented Jan 20, 2020

per discuss on slack, we'll keep two significant digits, e.g. 0.000016.

@leexgh
Copy link
Member Author

leexgh commented Jan 20, 2020

@jjgao @alisman @onursumer updated to 2 significant digits, could you take a look again? thanks

@alisman alisman merged commit 694a016 into cBioPortal:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants