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

Filter chooser model now accepts shorthand number inputs #3155

Merged
merged 4 commits into from
Oct 7, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

### 🐞 Bug Fixes

* FilterChooserModel now accepts numeric shorthand for numeric fields. (ie, '1,337k' => 1337000.0)
* Boolean filterChooser fields with enableValues=false will no longer suggest true for 'false'.
* Change to `CompoundTaskObserver` to prioritize using specific messages from subtasks over the
overall task message.
Expand Down
20 changes: 18 additions & 2 deletions cmp/filter/FilterChooserFieldSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
*/
import {BaseFilterFieldSpec} from '@xh/hoist/data/filter/BaseFilterFieldSpec';
import {FieldType, parseFieldValue} from '@xh/hoist/data';
import {fmtDate} from '@xh/hoist/format';
import {fmtDate, parseNumber} from '@xh/hoist/format';
import {stripTags, throwIf} from '@xh/hoist/utils/js';
import {isFunction, isNil} from 'lodash';
import {isValidElement} from 'react';
import {renderToStaticMarkup} from 'react-dom/server';

const {INT, NUMBER} = FieldType;

/**
* Filter field specification class for the typeahead `FilterChooser` component. Manages additional
Expand Down Expand Up @@ -47,7 +51,7 @@ export class FilterChooserFieldSpec extends BaseFilterFieldSpec {
super(rest);

this.valueRenderer = valueRenderer;
this.valueParser = valueParser;
this.valueParser = this.parseValueParser(valueParser);
this.example = this.parseExample(example);

if (!this.hasExplicitValues &&
Expand Down Expand Up @@ -77,6 +81,10 @@ export class FilterChooserFieldSpec extends BaseFilterFieldSpec {
let ret;
if (isFunction(this.valueRenderer)) {
ret = this.valueRenderer(value, op);
if (isValidElement(ret)) {
// Prevents [object Object] rendering
ret = renderToStaticMarkup(ret);
}
Copy link
Member

Choose a reason for hiding this comment

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

I was initially concerned that we were going to emit markup somewhere in the stack, given that the rendering pipeline for FilterChooser is fully React (as opposed to the issue handled by #3154 where we pipe into ag-grid context menus that require strings).

But that point is rendered moot 😁 by the use of stripTags below, which is where we first ended up with [object Object] before this fix.

I thought maybe we could update that to let this method return either a stripped string or an element, but we look to do string-based comparisons downstream so, yeah, we def. need this to return a string

} else if (this.isDateBasedFieldType) {
ret = fmtDate(value);
} else {
Expand Down Expand Up @@ -109,6 +117,14 @@ export class FilterChooserFieldSpec extends BaseFilterFieldSpec {
return 'value';
}

parseValueParser(valueParser) {
// Default numeric parser
if (!valueParser && (this.fieldType === INT || this.fieldType === NUMBER)) {
return (input, op) => parseNumber(input);
}
return valueParser;
}

loadValuesFromSource() {
const {field, source} = this,
values = new Set();
Expand Down
27 changes: 2 additions & 25 deletions desktop/cmp/input/NumberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import composeRefs from '@seznam/compose-react-refs';
import {HoistInputModel, HoistInputPropTypes, useHoistInputModel} from '@xh/hoist/cmp/input';
import {hoistCmp} from '@xh/hoist/core';
import '@xh/hoist/desktop/register';
import {fmtNumber} from '@xh/hoist/format';
import {fmtNumber, parseNumber} from '@xh/hoist/format';
import {numericInput} from '@xh/hoist/kit/blueprint';
import {wait} from '@xh/hoist/promise';
import {debounced, throwIf, withDefault} from '@xh/hoist/utils/js';
Expand Down Expand Up @@ -127,8 +127,6 @@ NumberInput.hasLayoutSupport = true;
//-----------------------
class NumberInputModel extends HoistInputModel {

static shorthandValidator = /((\.\d+)|(\d+(\.\d+)?))([kmb])\b/i;

constructor() {
super();
throwIf(Math.log10(this.scaleFactor) % 1 !== 0, 'scaleFactor must be a factor of 10');
Expand Down Expand Up @@ -213,28 +211,7 @@ class NumberInputModel extends HoistInputModel {
}

parseValue(value) {
if (isNil(value) || value === '') return null;

value = value.toString();
value = value.replace(/,/g, '');

if (NumberInputModel.shorthandValidator.test(value)) {
const num = +value.substring(0, value.length - 1),
lastChar = value.charAt(value.length - 1).toLowerCase();

switch (lastChar) {
case 'k':
return num * 1000;
case 'm':
return num * 1000000;
case 'b':
return num * 1000000000;
default:
return NaN;
}
}

return parseFloat(value);
return parseNumber(value);
}

noteFocused() {
Expand Down
34 changes: 33 additions & 1 deletion format/FormatNumber.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Copyright © 2022 Extremely Heavy Industries Inc.
*/
import {span} from '@xh/hoist/cmp/layout';
import {defaults, isFinite, isFunction, isPlainObject, isString} from 'lodash';
import {defaults, isFinite, isFunction, isNil, isPlainObject, isString} from 'lodash';
import numbro from 'numbro';
import {fmtSpan} from './FormatMisc';
import {createRenderer, saveOriginal} from './FormatUtils';
Expand Down Expand Up @@ -369,6 +369,38 @@ export const numberRenderer = createRenderer(fmtNumber),
priceRenderer = createRenderer(fmtPrice),
percentRenderer = createRenderer(fmtPercent);


const shorthandValidator = /((\.\d+)|(\d+(\.\d+)?))([kmb])\b/i;

/**
* @param {string} value - A string that represents a shorthand numerical value
* @returns {number} - The number represented by the shorthand string, or NaN
*/
export function parseNumber(value) {
if (isNil(value) || value === '') return null;

value = value.toString();
value = value.replace(/,/g, '');

if (shorthandValidator.test(value)) {
const num = +value.substring(0, value.length - 1),
lastChar = value.charAt(value.length - 1).toLowerCase();

switch (lastChar) {
case 'k':
return num * 1000;
case 'm':
return num * 1000000;
case 'b':
return num * 1000000000;
default:
return NaN;
}
}

return parseFloat(value);
}

/**
* @callback fmtNumber~tooltipFn - renderer for a custom tooltip.
* @param {number} originalValue - number to be formatted.
Expand Down