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

mrc-2818 Use formats in GenericChartTable #649

Merged
merged 5 commits into from
Dec 17, 2021
Merged

mrc-2818 Use formats in GenericChartTable #649

merged 5 commits into from
Dec 17, 2021

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Dec 15, 2021

Description

This branch uses the valueFormatColumn defined in chart configuration to display formatted values in the Input Time Series table (GenericChartTable). This allows us to show percentage-formatted values e.g. for ART proportion < 15 and ANC prevalence.

This format from the column metadata was already being used in the Plotly chart, so this branch pulls out the code which fetched that format from the selected option of the valueFormatColumn, into a new computed property, so it can be passed as a prop to GenericChartTable as well as being passed to Plotly.

In GenericChartTable this format is applied to any columns configured to useValueFormat.

My first attempt at this used numeral.js to format the values, as we do elsewhere. However, this caused some discrepancies between the formatted values displayed in Plotly and in the table - the first value visible at District level for Malwi (Chitipa, 2011) for ANC prevalence was displayed as 3.1% in the plot and 3.2% in the table - the actual value is 0.0315, so it's being rounded down by Plotly and rounded up by Numeral.js. This isn't the case with all values ending in '5' - for Chitipa's 2018 ANC prevalence value, 0.0245, both Plotly and Numeral.js round this to 2.5%. This is likely connected to this discussion of the issue - d3-format, used in Plotly, is using toFixed which can give unexpected results due to the imprecise nature of floating point representation. Numeral.js is presumably implemented in a different enough way to give slightly different results on occasion.

It didn't seem acceptable to have the same values rounded differently in chart and accompanying table, so now the GenericChartTable is using d3-format to format the values in order to be consistent with Plotly. The formats still arrive in the metadata as numeral.js format, as this is the standard in our metadata - but for both Plotly and GenericChartTable we convert them into d3 formats before use.

Type of version change

Delete as appropriate

Minor

Checklist

  • I have incremented version number, or version needs no increment
  • The build passed successfully, or failed because of ADR tests

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #649 (8e3155e) into master (fc9d21e) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #649   +/-   ##
=======================================
  Coverage        98%    98%           
  Complexity      282    282           
=======================================
  Files           208    208           
  Lines          6136   6141    +5     
  Branches        865    868    +3     
=======================================
+ Hits           6035   6040    +5     
  Misses           66     66           
  Partials         35     35           
Impacted Files Coverage Δ
...c/src/app/components/genericChart/GenericChart.vue 100% <100%> (ø)
.../app/components/genericChart/GenericChartTable.vue 100% <100%> (ø)
src/app/static/src/app/components/plots/utils.ts 100% <100%> (ø)
src/app/static/src/app/hintVersion.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc9d21e...8e3155e. Read the comment docs.

@EmmaLRussell EmmaLRussell marked this pull request as ready for review December 16, 2021 09:59
Copy link
Contributor

@LekanAnni LekanAnni left a comment

Choose a reason for hiding this comment

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

Works well...

See inline for a tiny suggestion .

Comment on lines 248 to 249
const numeralJsFormat = (selections[valueFormatColumn][0] as GenericChartColumnValue).format || "";
return numeralJsFormat ? numeralJsToD3format(numeralJsFormat) : "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const numeralJsFormat = (selections[valueFormatColumn][0] as GenericChartColumnValue).format || "";
return numeralJsFormat ? numeralJsToD3format(numeralJsFormat) : "";
const numeralJsFormat = (selections[valueFormatColumn][0] as GenericChartColumnValue)?.format;
if(numeralJsFormat) {
return numeralJsToD3format(numeralJsFormat)
}

Copy link
Contributor

@LekanAnni LekanAnni left a comment

Choose a reason for hiding this comment

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

LGTM

@EmmaLRussell EmmaLRussell merged commit 2443d5d into master Dec 17, 2021
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