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

feat: Picker - formatter settings #1907

Merged
merged 10 commits into from
Apr 3, 2024
1 change: 1 addition & 0 deletions packages/jsapi-components/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export { default as useDebouncedViewportSearch } from './useDebouncedViewportSea
export * from './useDebouncedViewportSelectionFilter';
export * from './useFilterConditionFactories';
export * from './useFilteredItemsWithDefaultValue';
export * from './useFormatter';
export * from './useGetItemIndexByValue';
export * from './useGetItemPosition';
export { default as useInitializeViewportData } from './useInitializeViewportData';
Expand Down
16 changes: 7 additions & 9 deletions packages/jsapi-components/src/spectrum/Picker/Picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import {
Picker as PickerBase,
PickerProps as PickerPropsBase,
} from '@deephaven/components';
import { useApi } from '@deephaven/jsapi-bootstrap';
import { dh as DhType } from '@deephaven/jsapi-types';
import { Formatter } from '@deephaven/jsapi-utils';
import { Settings } from '@deephaven/jsapi-utils';
import Log from '@deephaven/log';
import { PICKER_ITEM_HEIGHT, PICKER_TOP_OFFSET } from '@deephaven/utils';
import { useCallback, useEffect, useMemo } from 'react';
import useFormatter from '../../useFormatter';
import useGetItemIndexByValue from '../../useGetItemIndexByValue';
import { useViewportData } from '../../useViewportData';
import { getPickerKeyColumn } from './PickerUtils';
Expand All @@ -24,21 +24,19 @@ export interface PickerProps extends Omit<PickerPropsBase, 'children'> {
labelColumn?: string;

// TODO #1890 : descriptionColumn, iconColumn

settings?: Settings;
}

export function Picker({
table,
keyColumn: keyColumnName,
labelColumn: labelColumnName,
selectedKey,
settings,
...props
}: PickerProps): JSX.Element {
const dh = useApi();

const formatValue = useMemo(() => {
const formatter = new Formatter(dh);
return formatter.getFormattedString.bind(formatter);
}, [dh]);
const { getFormattedString: formatValue } = useFormatter(settings);

const keyColumn = useMemo(
() => getPickerKeyColumn(table, keyColumnName),
Expand Down Expand Up @@ -100,7 +98,7 @@ export function Picker({
isCanceled = true;
};
},
[getItemIndexByValue, setViewport]
[getItemIndexByValue, settings, setViewport]
);

return (
Expand Down
69 changes: 69 additions & 0 deletions packages/jsapi-components/src/useFormatter.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { useApi } from '@deephaven/jsapi-bootstrap';
import { bindAllMethods, TestUtils } from '@deephaven/utils';
import {
createFormatterFromSettings,
Formatter,
Settings,
} from '@deephaven/jsapi-utils';
import type { dh as DhType } from '@deephaven/jsapi-types';
import { renderHook } from '@testing-library/react-hooks';
import { useFormatter } from './useFormatter';

jest.mock('@deephaven/jsapi-bootstrap');
jest.mock('@deephaven/jsapi-utils', () => {
const actual = jest.requireActual('@deephaven/jsapi-utils');
return {
...actual,
createFormatterFromSettings: jest.fn(),
};
});
jest.mock('@deephaven/utils', () => ({
...jest.requireActual('@deephaven/utils'),
bindAllMethods: jest.fn(),
}));

const { asMock, createMockProxy } = TestUtils;

beforeEach(() => {
jest.clearAllMocks();
expect.hasAssertions();
});

describe('useFormatter', () => {
const mock = {
dh: createMockProxy<typeof DhType>(),
formatter: createMockProxy<Formatter>(),
settings: createMockProxy<Settings>(),
};

beforeEach(() => {
asMock(useApi).mockReturnValue(mock.dh);

asMock(bindAllMethods)
.mockName('bindAllMethods')
.mockImplementation(a => a);

asMock(createFormatterFromSettings)
.mockName('createFormatterFromSettings')
.mockReturnValue(mock.formatter);
});

it('should return members of a `Formatter` instance based on settings', () => {
const { result } = renderHook(() => useFormatter(mock.settings));

expect(createFormatterFromSettings).toHaveBeenCalledWith(
mock.dh,
mock.settings
);

expect(bindAllMethods).toHaveBeenCalledWith(mock.formatter);

expect(result.current).toEqual({
getColumnFormat: mock.formatter.getColumnFormat,
getColumnFormatMapForType: mock.formatter.getColumnFormatMapForType,
getColumnTypeFormatter: mock.formatter.getColumnTypeFormatter,
getFormattedString: mock.formatter.getFormattedString,
timeZone: mock.formatter.timeZone,
});
});
});
55 changes: 55 additions & 0 deletions packages/jsapi-components/src/useFormatter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { useApi } from '@deephaven/jsapi-bootstrap';
import {
createFormatterFromSettings,
Formatter,
Settings,
} from '@deephaven/jsapi-utils';
import { bindAllMethods } from '@deephaven/utils';
import { useMemo } from 'react';

export type UseFormatterResult = Pick<
Formatter,
| 'getColumnFormat'
| 'getColumnFormatMapForType'
| 'getColumnTypeFormatter'
| 'getFormattedString'
| 'timeZone'
>;

/**
* Returns a subset of members of a `Formatter` instance. The `Formatter` will be
* constructed based on the given options or fallback to the configuration found
* in the current `FormatSettingsContext`. Members that are functions are bound
* to the `Formatter` instance, so they are safe to destructure. Static methods
* can still be accessed statically from the `Formatter` class.
* @param settings Optional settings to use when constructing the `Formatter`
*/
export function useFormatter(settings?: Settings): UseFormatterResult {
const dh = useApi();

const formatter = useMemo(() => {
const instance = createFormatterFromSettings(dh, settings);

// Bind all methods so we can destructure them
bindAllMethods(instance);

return instance;
}, [dh, settings]);

const {
getColumnFormat,
getColumnFormatMapForType,
getColumnTypeFormatter,
getFormattedString,
} = formatter;

return {
getColumnFormat,
getColumnFormatMapForType,
getColumnTypeFormatter,
getFormattedString,
timeZone: formatter.timeZone,
};
}

export default useFormatter;
14 changes: 14 additions & 0 deletions packages/jsapi-components/src/useGetItemIndexByValue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,20 @@ describe('useGetItemIndexByValue', () => {
}
);

it('should throw if seekRow fails', async () => {
asMock(mockTable.seekRow).mockRejectedValue('Some error');

const { result } = renderHook(() =>
useGetItemIndexByValue({
columnName: 'mock.columnName',
table: mockTable,
value: 'mock.value',
})
);

expect(result.current()).rejects.toEqual('Some error');
});

it.each([
['mock.value', 42, 42],
['mock.value', -1, null],
Expand Down
13 changes: 10 additions & 3 deletions packages/jsapi-components/src/useGetItemIndexByValue.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { useCallback } from 'react';
import { dh } from '@deephaven/jsapi-types';
import Log from '@deephaven/log';
import { useTableUtils } from './useTableUtils';

const log = Log.module('useGetItemIndexByValue');

/**
* Returns a function that gets the index of the first row containing a column
* value.
Expand Down Expand Up @@ -30,9 +33,13 @@ export function useGetItemIndexByValue<TValue>({
const column = table.findColumn(columnName);
const columnValueType = tableUtils.getValueType(column.type);

const index = await table.seekRow(0, column, columnValueType, value);

return index === -1 ? null : index;
try {
const index = await table.seekRow(0, column, columnValueType, value);
return index === -1 ? null : index;
} catch (err) {
log.debug('Error seeking row', { column, value, columnValueType });
throw err;
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't bother logging it either - consumer either catches it and logs it or does something with it in which case the log would be extra spam. If it's uncaught would be logged as an uncaught promise anyway.

}
}, [columnName, table, tableUtils, value]);
}

Expand Down
71 changes: 69 additions & 2 deletions packages/jsapi-utils/src/FormatterUtils.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import Formatter from './Formatter';
import dh from '@deephaven/jsapi-shim';
import { TestUtils } from '@deephaven/utils';
import Formatter, { FormattingRule } from './Formatter';
import FormatterUtils, {
createFormatterFromSettings,
getColumnFormats,
getDateTimeFormatterOptions,
} from './FormatterUtils';
Expand All @@ -9,7 +12,31 @@ import {
TableColumnFormatType,
} from './formatters';
import TableUtils from './TableUtils';
import { ColumnFormatSettings, DateTimeFormatSettings } from './Settings';
import Settings, {
ColumnFormatSettings,
DateTimeFormatSettings,
} from './Settings';

jest.mock('./Formatter', () => {
const FormatterActual = jest.requireActual('./Formatter').default;

// Extract static methods
const { makeColumnFormatMap, makeColumnFormattingRule } = FormatterActual;

// Wrap Formatter constructor so we can spy on it
const mockFormatter = jest.fn((...args) => new FormatterActual(...args));

return {
__esModule: true,
default: Object.assign(mockFormatter, {
makeColumnFormatMap,
makeColumnFormattingRule,
}),
};
});

const { createMockProxy } = TestUtils;
const FormatterActual = jest.requireActual('./Formatter').default;

function makeFormatter(...settings: ConstructorParameters<typeof Formatter>) {
return new Formatter(...settings);
Expand All @@ -23,6 +50,46 @@ function makeFormattingRule(
return { label, formatString, type };
}

describe('createFormatterFromSettings', () => {
const mockSettings = createMockProxy<Settings>({
formatter: [
createMockProxy<FormattingRule>({
format: createMockProxy<TableColumnFormat>(),
}),
],
});

it.each([undefined, mockSettings])(
'should create a formatter with the given settings: %s',
settings => {
const columnRules = getColumnFormats(settings);
const dateTimeOptions = getDateTimeFormatterOptions(settings);

const {
defaultDecimalFormatOptions,
defaultIntegerFormatOptions,
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings,
} = settings ?? {};

const actual = createFormatterFromSettings(dh, settings);

expect(Formatter).toHaveBeenCalledWith(
dh,
columnRules,
dateTimeOptions,
defaultDecimalFormatOptions,
defaultIntegerFormatOptions,
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings
);
expect(actual).toBeInstanceOf(FormatterActual);
}
);
});

describe('isCustomColumnFormatDefined', () => {
const columnFormats = [
Formatter.makeColumnFormattingRule(
Expand Down
40 changes: 39 additions & 1 deletion packages/jsapi-utils/src/FormatterUtils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,44 @@
import type { dh as DhType } from '@deephaven/jsapi-types';
import type { FormattingRule } from './Formatter';
import Formatter from './Formatter';
import { DateTimeColumnFormatter, TableColumnFormatter } from './formatters';
import { ColumnFormatSettings, DateTimeFormatSettings } from './Settings';
import Settings, {
ColumnFormatSettings,
DateTimeFormatSettings,
} from './Settings';

/**
* Instantiate a `Formatter` from the given settings.
* @param dh The `dh` object
* @param settings Optional settings to use
* @returns A new `Formatter` instance
*/
export function createFormatterFromSettings(
dh: typeof DhType,
settings?: Settings
): Formatter {
const columnRules = getColumnFormats(settings);
const dateTimeOptions = getDateTimeFormatterOptions(settings);

const {
defaultDecimalFormatOptions,
defaultIntegerFormatOptions,
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings,
} = settings ?? {};

return new Formatter(
dh,
columnRules,
dateTimeOptions,
defaultDecimalFormatOptions,
defaultIntegerFormatOptions,
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings
);
}

export function getColumnFormats(
settings?: ColumnFormatSettings
Expand Down Expand Up @@ -45,6 +82,7 @@ export function isCustomColumnFormatDefined(
}

export default {
createFormatterFromSettings,
getColumnFormats,
getDateTimeFormatterOptions,
isCustomColumnFormatDefined,
Expand Down
Loading
Loading