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(helpers): ent-4366 optional chaining for numbro #832

Merged
merged 2 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/common/__tests__/__snapshots__/helpers.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Helpers should apply a number display function: number display function result 1`] = `
t {
"_value": 11,
}
`;

exports[`Helpers should expose a window object: limited window object 1`] = `
Object {
"DEV_MODE": false,
Expand Down Expand Up @@ -35,6 +41,7 @@ Object {
"noop": [Function],
"noopPromise": Promise {},
"noopTranslate": [Function],
"numberDisplay": [Function],
}
`;

Expand Down Expand Up @@ -73,6 +80,7 @@ Object {
"noop": [Function],
"noopPromise": Promise {},
"noopTranslate": [Function],
"numberDisplay": [Function],
}
`;

Expand Down Expand Up @@ -110,5 +118,6 @@ Object {
"noop": [Function],
"noopPromise": Promise {},
"noopTranslate": [Function],
"numberDisplay": [Function],
}
`;
7 changes: 7 additions & 0 deletions src/common/__tests__/helpers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ describe('Helpers', () => {
expect(helpers.isPromise(() => 'lorem')).toBe(false);
});

it('should apply a number display function', () => {
expect(helpers.numberDisplay(null)).toBe(null);
expect(helpers.numberDisplay(undefined)).toBe(undefined);
expect(helpers.numberDisplay(NaN)).toBe(NaN);
expect(helpers.numberDisplay(11)).toMatchSnapshot('number display function result');
});

it('should expose a window object', () => {
helpers.browserExpose({ lorem: 'ipsum' });
expect(window[helpers.UI_WINDOW_ID]).toMatchSnapshot('window object');
Expand Down
20 changes: 20 additions & 0 deletions src/common/helpers.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import numbro from 'numbro';

/**
* Generate a random'ish ID.
*
Expand Down Expand Up @@ -60,6 +62,23 @@ const noopTranslate = (key, value, components) => {
})`;
};

/**
* ToDo: review adding "locale" for numbro
*/
/**
* Convenience wrapper for numbro. Numbro assumes all values passed to it conform as "number".
* This allows us to optional chain the function results.
*
* @param {*} value
* @returns {numbro.Numbro|*}
*/
const numberDisplay = value => {
if (typeof value !== 'number' || Number.isNaN(value)) {
return value;
}
return numbro(value);
};

/**
* Is dev mode active.
* Associated with using the NPM script "start". See dotenv config files for activation.
Expand Down Expand Up @@ -277,6 +296,7 @@ const helpers = {
noop,
noopPromise,
noopTranslate,
numberDisplay,
DEV_MODE,
PROD_MODE,
REVIEW_MODE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ exports[`GraphCardMetricTotals Component should handle multiple display states:
>
<CardBody>
<div>
t(curiosity-graph.cardBodyMetric_total, {"context":"","total":"NAN"}, [object Object])
t(curiosity-graph.cardBodyMetric_total, {"context":""}, [object Object])
</div>
</CardBody>
</MinHeight>
Expand Down Expand Up @@ -177,7 +177,7 @@ exports[`GraphCardMetricTotals Component should handle multiple display states:
>
<CardBody>
<div>
t(curiosity-graph.cardBodyMetric_total, {"context":"","total":"NAN"}, [object Object])
t(curiosity-graph.cardBodyMetric_total, {"context":""}, [object Object])
</div>
</CardBody>
</MinHeight>
Expand Down
9 changes: 5 additions & 4 deletions src/components/graphCard/graphCardChartTooltip.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React from 'react';
import PropTypes from 'prop-types';
import numbro from 'numbro';
import { useProduct, useProductGraphTallyQuery } from '../productView/productViewContext';
import { getTooltipDate } from './graphCardHelpers';
import { translate } from '../i18n/i18n';
import { ChartIcon } from '../chart/chartIcon';
import { RHSM_API_QUERY_SET_TYPES } from '../../services/rhsm/rhsmConstants';
import { helpers } from '../../common';

/**
* A custom chart tooltip.
Expand Down Expand Up @@ -93,9 +93,10 @@ const GraphCardChartTooltip = ({
const updatedDataFacetValue =
(typeof dataFacet.value === 'number' &&
!Number.isInteger(dataFacet.value) &&
numbro(dataFacet.value)
.format({ average: true, mantissa: 5, trimMantissa: true, lowPrecision: true })
.toUpperCase()) ||
helpers
.numberDisplay(dataFacet.value)
?.format({ average: true, mantissa: 5, trimMantissa: true, lowPrecision: true })
?.toUpperCase()) ||
dataFacet.value;

return (
Expand Down
14 changes: 5 additions & 9 deletions src/components/graphCard/graphCardHelpers.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import moment from 'moment';
import { chart_color_green_300 as chartColorGreenDark } from '@patternfly/react-tokens';
import numbro from 'numbro';
import { RHSM_API_QUERY_GRANULARITY_TYPES as GRANULARITY_TYPES } from '../../types/rhsmApiTypes';
import { dateHelpers } from '../../common';
import { dateHelpers, helpers } from '../../common';

/**
* Update chart/graph filters with base settings with styling.
Expand Down Expand Up @@ -156,10 +155,6 @@ const xAxisTickFormat = ({ callback, date, granularity, tick, previousDate } = {
return formattedDate;
};

/**
* ToDo: review adding "locale" for numbro
* Original settings, numbro(tick).format({ average: true, mantissa: 1, optionalMantissa: true });
*/
/**
* Format y axis ticks.
*
Expand All @@ -173,14 +168,15 @@ const yAxisTickFormat = ({ callback, tick } = {}) => {
return callback({ tick });
}

return numbro(tick)
.format({
return helpers
.numberDisplay(tick)
?.format({
average: true,
mantissa: 1,
trimMantissa: true,
lowPrecision: false
})
.toUpperCase();
?.toUpperCase();
};

/**
Expand Down
22 changes: 14 additions & 8 deletions src/components/graphCard/graphCardMetricTotals.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ import React from 'react';
import PropTypes from 'prop-types';
import { Card, CardBody, CardFooter, CardTitle, Flex, FlexItem } from '@patternfly/react-core';
import moment from 'moment';
import numbro from 'numbro';
import { useProductGraphTallyQuery } from '../productView/productViewContext';
import { useMetricsSelector } from './graphCardContext';
import { MinHeight } from '../minHeight/minHeight';
import { Loader, SkeletonSize } from '../loader/loader';
import { dateHelpers } from '../../common';
import { dateHelpers, helpers } from '../../common';
import { toolbarFieldOptions } from '../toolbar/toolbarFieldRangedMonthly';
import { RHSM_API_QUERY_SET_TYPES } from '../../services/rhsm/rhsmConstants';
import { translate } from '../i18n/i18n';
Expand Down Expand Up @@ -66,9 +65,15 @@ const GraphCardMetricTotals = ({
'curiosity-graph.cardBodyMetric_total',
{
context: (dailyHasData && metricId) || '',
total: numbro(dailyValue)
.format({ average: true, mantissa: 5, trimMantissa: true, lowPrecision: false })
.toUpperCase()
total: helpers
.numberDisplay(dailyValue)
?.format({
average: true,
mantissa: 5,
trimMantissa: true,
lowPrecision: false
})
?.toUpperCase()
},
[<strong title={dailyValue} aria-label={dailyValue} />]
)}
Expand Down Expand Up @@ -103,9 +108,10 @@ const GraphCardMetricTotals = ({
'curiosity-graph.cardBodyMetric_total',
{
context: (monthlyHasData && metricId) || '',
total: numbro(monthlyValue)
.format({ average: true, mantissa: 5, trimMantissa: true, lowPrecision: false })
.toUpperCase()
total: helpers
.numberDisplay(monthlyValue)
?.format({ average: true, mantissa: 5, trimMantissa: true, lowPrecision: false })
?.toUpperCase()
},
[<strong title={monthlyValue} aria-label={monthlyValue} />]
)}
Expand Down
4 changes: 2 additions & 2 deletions src/components/i18n/__tests__/__snapshots__/i18n.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ Array [
"keys": Array [
Object {
"key": "curiosity-graph.cardActionTotal",
"match": "translate('curiosity-graph.cardActionTotal', { context: 'coreHours', total: numbro(totalCoreHours)",
"match": "translate('curiosity-graph.cardActionTotal', { context: 'coreHours', total: helpers .numberDisplay(totalCoreHours)",
},
Object {
"key": "curiosity-inventory.label",
Expand All @@ -567,7 +567,7 @@ Array [
"keys": Array [
Object {
"key": "curiosity-graph.cardActionTotal",
"match": "translate('curiosity-graph.cardActionTotal', { context: 'coreHours', total: numbro(totalCoreHours)",
"match": "translate('curiosity-graph.cardActionTotal', { context: 'coreHours', total: helpers .numberDisplay(totalCoreHours)",
},
Object {
"key": "curiosity-inventory.label",
Expand Down
Loading