diff --git a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor-matrix.stories.ts b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor-matrix.stories.ts index b9af53fc3d..f870e2a1ae 100644 --- a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor-matrix.stories.ts +++ b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor-matrix.stories.ts @@ -14,6 +14,7 @@ import { controlLabelFont, controlLabelFontColor } from '../../../theme-provider/design-tokens'; +import { placeholderStates, type PlaceholderState } from '../../../utilities/tests/states'; const metadata: Meta = { title: 'Tests/Table Column: Anchor', @@ -50,18 +51,12 @@ const appearanceStates: [string, string | undefined][] = Object.entries( ).map(([key, value]) => [pascalCase(key), value]); type AppearanceState = (typeof appearanceStates)[number]; -const underlineHiddenStates: [string, boolean][] = [ +const underlineHiddenStates = [ ['Underline Hidden', true], ['', false] -]; +] as const; type UnderlineHiddenState = (typeof underlineHiddenStates)[number]; -const placeholderStates: [string, string | undefined][] = [ - ['With Placeholder', 'Custom placeholder'], - ['', undefined] -]; -type PlaceholderState = (typeof placeholderStates)[number]; - // prettier-ignore const component = ( [appearanceName, appearance]: AppearanceState, diff --git a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx index 07f45c99d6..3b537c287f 100644 --- a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx +++ b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx @@ -16,18 +16,11 @@ The column is used to display string fields a ### Best Practices -- Provide useful labels for well known urls. While an absent label will show the full URL for accessibility, it is useful to instead provide a clear and unique label to improve grouping. - - For example, a column of links to notebooks where a notebook may no longer exist, and thus a label is not available, could pre-process the notebook urls and create the label `Missing Notebook (UNIQUE_NOTEBOOK_ID)`. This allows multiple rows referencing the same missing notebook to be grouped together. - - Alternatively if the urls are not well-known structures, the application should explicitly provide the href as the label to keep unique labels and preserve grouping as opposed to using `null` / `undefined` labels. -- Applications should avoid having duplicate labels to different hrefs as those are inaccessible to screen readers (and sighted users). See [high-level discussion](https://fae.disability.illinois.edu/rulesets/LINK_2/) of [aria SC 2.4.4](https://www.w3.org/TR/WCAG22/#link-purpose-in-context). - - For example, applications should avoid having `undefined` / `null` as the label as that causes multiple unrelated URLs to be grouped together under the group label "No value". Accessibility is okay as the full url will be shown but the value of grouping is limited. - - For example, if a label is missing, an application should avoid generating a non-unique label for multiple URLs (i.e. `Missing Notebook`) as that harms accessibility and limits the value of grouping. -- Avoid using empty string or other whitespace-only labels with defined hrefs. This will cause the rendered anchor to have no text associated with it, and it will be difficult for a user to see that the anchor exists. -- Applications may leave the href as `null` / `undefined` to have the anchor column behave effectively like a string column -- Avoid mixing `undefined` and `null` as values for the label field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. - - As explained above, it is not recommended to use `undefined` or `null` labels when the data has defined hrefs. -- Avoid mixing empty string with `undefined`/`null` as values for the label field. The distinction when grouping between `"No value"` and `"Empty"` is not likely meaningful to a user. - - As explained above, it is not recommended to use empty string, `undefined`, or `null` labels when the data has defined hrefs. +- Provide meaningful, distinct labels for records that have a non-empty url. + - Do not provide `null`, `undefined`, empty, or whitespace labels with non-empty urls. + - Do not provide duplicate labels for different urls. + - If a label is not available or known for a url, the url itself may be explicitly provided for the label to ensure each distinct url has a distinct label. +- For records without a url, the label may also be omitted, but the omitted label should be consistent in using `null`, `undefined`, or empty string. ### Target Configuration diff --git a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts index 5315967bf2..63503b5e3a 100644 --- a/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts +++ b/packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.spec.ts @@ -8,7 +8,6 @@ import { TableColumnSortDirection, TableRecord } from '../../../table/types'; import { TablePageObject } from '../../../table/testing/table.pageobject'; import { wackyStrings } from '../../../utilities/tests/wacky-strings'; import type { Anchor } from '../../../anchor'; -import { themeProviderTag } from '../../../theme-provider'; interface SimpleTableRecord extends TableRecord { label?: string | null; @@ -25,29 +24,27 @@ class ElementReferences { // prettier-ignore async function setup(source: ElementReferences): Promise>> { return fixture>( - html`<${themeProviderTag} lang="en-US"> - <${tableTag} style="width: 700px" ${ref('table')}> - <${tableColumnAnchorTag} - ${ref('column')} - label-field-name="label" - href-field-name="link" - appearance="prominent" - hreflang="hreflang value" - ping="ping value" - referrerpolicy="referrerpolicy value" - rel="rel value" - target="target value" - type="type value" - download="download value" - group-index="0" - > - Column 1 - - <${tableColumnAnchorTag}> - Column 2 - - - `, + html`<${tableTag} style="width: 700px" ${ref('table')}> + <${tableColumnAnchorTag} + ${ref('column')} + label-field-name="label" + href-field-name="link" + appearance="prominent" + hreflang="hreflang value" + ping="ping value" + referrerpolicy="referrerpolicy value" + rel="rel value" + target="target value" + type="type value" + download="download value" + group-index="0" + > + Column 1 + + <${tableColumnAnchorTag}> + Column 2 + + `, { source } ); } @@ -512,23 +509,34 @@ describe('TableColumnAnchor', () => { usesColumnPlaceholder: false }, { - name: 'label is not a string', + name: 'label is not a string with no href', data: [{ label: 10 as unknown as string }], cellValue: '', groupValue: '', usesColumnPlaceholder: false + }, + { + name: 'label is not a string with a defined href', + data: [{ label: 10 as unknown as string, link: 'link' }], + cellValue: 'link', + groupValue: '', + usesColumnPlaceholder: false } ]; + async function initializeColumnAndTable(data: readonly SimpleTableRecord[], placeholder?: string): Promise { + column.placeholder = placeholder; + await table.setData(data); + await connect(); + await waitForUpdatesAsync(); + } + parameterizeSpec(testCases, (spec, name, value) => { spec( `cell and group row render expected value when ${name} and placeholder is configured`, async () => { const placeholder = 'Custom placeholder'; - column.placeholder = placeholder; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data, placeholder); const expectedCellText = value.usesColumnPlaceholder ? placeholder @@ -547,9 +555,7 @@ describe('TableColumnAnchor', () => { spec( `cell and group row render expected value when ${name} and placeholder is not configured`, async () => { - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( value.cellValue @@ -563,10 +569,7 @@ describe('TableColumnAnchor', () => { it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { const placeholder = 'My placeholder'; - column.placeholder = placeholder; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder ); @@ -576,10 +579,8 @@ describe('TableColumnAnchor', () => { expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); }); - it('setting placeholder to defined string updates cells from displaying placeholder to displaying blank', async () => { - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + it('setting placeholder to defined string updates cells from displaying blank to displaying placeholder', async () => { + await initializeColumnAndTable([{}]); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); const placeholder = 'placeholder'; @@ -592,10 +593,7 @@ describe('TableColumnAnchor', () => { it('updating placeholder from one string to another updates cell', async () => { const placeholder1 = 'My first placeholder'; - column.placeholder = placeholder1; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder1); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder1 ); diff --git a/packages/nimble-components/src/table-column/base/tests/table-column-stories-utils.ts b/packages/nimble-components/src/table-column/base/tests/table-column-stories-utils.ts index 4cbdc261c8..84c6e6e7a5 100644 --- a/packages/nimble-components/src/table-column/base/tests/table-column-stories-utils.ts +++ b/packages/nimble-components/src/table-column/base/tests/table-column-stories-utils.ts @@ -52,3 +52,5 @@ export const sharedTableActions = [ ] as const; export const columnOperationBehavior = 'Column operations, such as sorting and grouping, are performed on the field values in the data records, not on the formatted values displayed within the cells.'; + +export const noNullAndUndefinedBestPractice = 'Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`.' diff --git a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text-matrix.stories.ts b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text-matrix.stories.ts index 4ba0dd25c5..3d6c52c7ff 100644 --- a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text-matrix.stories.ts +++ b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text-matrix.stories.ts @@ -12,6 +12,7 @@ import { controlLabelFont, controlLabelFontColor } from '../../../theme-provider/design-tokens'; +import { placeholderStates, type PlaceholderState } from '../../../utilities/tests/states'; const metadata: Meta = { title: 'Tests/Table Column: Date Text', @@ -36,12 +37,6 @@ const data = [ } ] as const; -const placeholderStates: [string, string | undefined][] = [ - ['With Placeholder', 'Custom placeholder'], - ['', undefined] -]; -type PlaceholderState = (typeof placeholderStates)[number]; - // prettier-ignore const component = ( [placeholderName, placeholder]: PlaceholderState diff --git a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx index 3e61018be3..85e06cf2c1 100644 --- a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx +++ b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx @@ -20,7 +20,7 @@ The column is used to display date-time fie ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. +- {noNullAndUndefinedBestPractice} {/* ## Examples */} diff --git a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.spec.ts b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.spec.ts index 11710adede..aec236b804 100644 --- a/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.spec.ts +++ b/packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.spec.ts @@ -476,189 +476,177 @@ describe('TableColumnDateText', () => { }); describe('placeholder', () => { - describe('placeholder', () => { - const testCases = [ - { - name: 'value is not specified', - data: [{}], - cellValue: '', - groupValue: 'No value', - usesColumnPlaceholder: true - }, - { - name: 'value is undefined', - data: [{ field: undefined }], - cellValue: '', - groupValue: 'No value', - usesColumnPlaceholder: true - }, - { - name: 'value is null', - data: [{ field: null }], - cellValue: '', - groupValue: 'No value', - usesColumnPlaceholder: true - }, - { - name: 'value is Number.NaN', - data: [{ field: Number.NaN }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is valid and non-zero', - data: [{ field: 1708984169258 }], - cellValue: '2/26/2024', - groupValue: '2/26/2024', - usesColumnPlaceholder: false - }, - { - name: 'value is incorrect type', - data: [{ field: 'not a number' as unknown as number }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is specified and falsey', - data: [{ field: 0 }], - cellValue: '1/1/1970', - groupValue: '1/1/1970', - usesColumnPlaceholder: false - }, - { - name: 'value is Inf', - data: [{ field: Number.POSITIVE_INFINITY }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is -Inf', - data: [{ field: Number.NEGATIVE_INFINITY }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is MAX_VALUE', - data: [{ field: Number.MAX_VALUE }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is too large for Date', - data: [{ field: 8640000000000000 + 1 }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false - }, - { - name: 'value is too small for Date', - data: [{ field: -8640000000000000 - 1 }], - cellValue: '', - groupValue: '', - usesColumnPlaceholder: false + const testCases = [ + { + name: 'value is not specified', + data: [{}], + cellValue: '', + groupValue: 'No value', + usesColumnPlaceholder: true + }, + { + name: 'value is undefined', + data: [{ field: undefined }], + cellValue: '', + groupValue: 'No value', + usesColumnPlaceholder: true + }, + { + name: 'value is null', + data: [{ field: null }], + cellValue: '', + groupValue: 'No value', + usesColumnPlaceholder: true + }, + { + name: 'value is Number.NaN', + data: [{ field: Number.NaN }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is valid and non-zero', + data: [{ field: 1708984169258 }], + cellValue: '2/26/2024', + groupValue: '2/26/2024', + usesColumnPlaceholder: false + }, + { + name: 'value is incorrect type', + data: [{ field: 'not a number' as unknown as number }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is specified and falsey', + data: [{ field: 0 }], + cellValue: '1/1/1970', + groupValue: '1/1/1970', + usesColumnPlaceholder: false + }, + { + name: 'value is Inf', + data: [{ field: Number.POSITIVE_INFINITY }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is -Inf', + data: [{ field: Number.NEGATIVE_INFINITY }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is MAX_VALUE', + data: [{ field: Number.MAX_VALUE }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is too large for Date', + data: [{ field: 8640000000000000 + 1 }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + }, + { + name: 'value is too small for Date', + data: [{ field: -8640000000000000 - 1 }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false + } + ]; + + async function initializeColumnAndTable(data: readonly SimpleTableRecord[], placeholder?: string): Promise { + // Set a custom time zone so that the behavior of the test does not + // depend on the configuration of the computer running the tests. + column.format = DateTextFormat.custom; + column.customTimeZone = 'UTC'; + column.placeholder = placeholder; + await table.setData(data); + await connect(); + await waitForUpdatesAsync(); + } + + parameterizeSpec(testCases, (spec, name, value) => { + spec( + `cell and group row render expected value when ${name} and placeholder is configured`, + async () => { + const placeholder = 'Custom placeholder'; + await initializeColumnAndTable(value.data, placeholder); + + const expectedCellText = value.usesColumnPlaceholder + ? placeholder + : value.cellValue; + expect( + pageObject.getRenderedCellContent(0, 0) + ).toBe(expectedCellText); + expect( + pageObject.getRenderedGroupHeaderContent(0) + ).toBe(value.groupValue); + } + ); + }); + + parameterizeSpec(testCases, (spec, name, value) => { + spec( + `cell and group row render expected value when ${name} and placeholder is not configured`, + async () => { + await initializeColumnAndTable(value.data); + + expect( + pageObject.getRenderedCellContent(0, 0) + ).toBe(value.cellValue); + expect( + pageObject.getRenderedGroupHeaderContent(0) + ).toBe(value.groupValue); } - ]; - - parameterizeSpec(testCases, (spec, name, value) => { - spec( - `cell and group row render expected value when ${name} and placeholder is configured`, - async () => { - // Set a custom time zone so that the behavior of the test does not - // depend on the configuration of the computer running the tests. - column.format = DateTextFormat.custom; - column.customTimeZone = 'UTC'; - const placeholder = 'Custom placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); - - const expectedCellText = value.usesColumnPlaceholder - ? placeholder - : value.cellValue; - expect( - pageObject.getRenderedCellContent(0, 0) - ).toBe(expectedCellText); - expect( - pageObject.getRenderedGroupHeaderContent(0) - ).toBe(value.groupValue); - } - ); - }); - - parameterizeSpec(testCases, (spec, name, value) => { - spec( - `cell and group row render expected value when ${name} and placeholder is not configured`, - async () => { - // Set a custom time zone so that the behavior of the test does not - // depend on the configuration of the computer running the tests. - column.format = DateTextFormat.custom; - column.customTimeZone = 'UTC'; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); - - expect( - pageObject.getRenderedCellContent(0, 0) - ).toBe(value.cellValue); - expect( - pageObject.getRenderedGroupHeaderContent(0) - ).toBe(value.groupValue); - } - ); - }); - - it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { - const placeholder = 'My placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe( - placeholder - ); - - elementReferences.column1.placeholder = undefined; - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); - }); - - it('setting placeholder to defined string updates cells from displaying placeholder to displaying blank', async () => { - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); - - const placeholder = 'placeholder'; - elementReferences.column1.placeholder = placeholder; - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe( - placeholder - ); - }); - - it('updating placeholder from one string to another updates cell', async () => { - const placeholder1 = 'My first placeholder'; - elementReferences.column1.placeholder = placeholder1; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe( - placeholder1 - ); - - const placeholder2 = 'My second placeholder'; - elementReferences.column1.placeholder = placeholder2; - await waitForUpdatesAsync(); - expect(pageObject.getRenderedCellContent(0, 0)).toBe( - placeholder2 - ); - }); + ); + }); + + it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { + const placeholder = 'My placeholder'; + await initializeColumnAndTable([{}], placeholder); + expect(pageObject.getRenderedCellContent(0, 0)).toBe( + placeholder + ); + + column.placeholder = undefined; + await waitForUpdatesAsync(); + expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); + }); + + it('setting placeholder to defined string updates cells from displaying blank to displaying placeholder', async () => { + await initializeColumnAndTable([{}]); + expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); + + const placeholder = 'placeholder'; + column.placeholder = placeholder; + await waitForUpdatesAsync(); + expect(pageObject.getRenderedCellContent(0, 0)).toBe( + placeholder + ); + }); + + it('updating placeholder from one string to another updates cell', async () => { + const placeholder1 = 'My first placeholder'; + await initializeColumnAndTable([{}], placeholder1); + expect(pageObject.getRenderedCellContent(0, 0)).toBe( + placeholder1 + ); + + const placeholder2 = 'My second placeholder'; + column.placeholder = placeholder2; + await waitForUpdatesAsync(); + expect(pageObject.getRenderedCellContent(0, 0)).toBe( + placeholder2 + ); }); }); }); diff --git a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text-matrix.stories.ts b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text-matrix.stories.ts index 66cf14f3ca..fd355befe0 100644 --- a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text-matrix.stories.ts +++ b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text-matrix.stories.ts @@ -11,6 +11,7 @@ import { controlLabelFont, controlLabelFontColor } from '../../../theme-provider/design-tokens'; +import { placeholderStates, type PlaceholderState } from '../../../utilities/tests/states'; const metadata: Meta = { title: 'Tests/Table Column: Duration Text', @@ -35,12 +36,6 @@ const data = [ } ] as const; -const placeholderStates: [string, string | undefined][] = [ - ['With Placeholder', 'Custom placeholder'], - ['', undefined] -]; -type PlaceholderState = (typeof placeholderStates)[number]; - // prettier-ignore const component = ( [placeholderName, placeholder]: PlaceholderState diff --git a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.mdx b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.mdx index e97cad48ce..1f7ad7da18 100644 --- a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.mdx +++ b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.mdx @@ -18,7 +18,7 @@ The column is used to display a period ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. +- {noNullAndUndefinedBestPractice} ### Angular Usage diff --git a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.spec.ts b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.spec.ts index 2a55ee08a7..a519fc8328 100644 --- a/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.spec.ts +++ b/packages/nimble-components/src/table-column/duration-text/tests/table-column-duration-text.spec.ts @@ -247,15 +247,19 @@ describe('TableColumnDurationText', () => { } ]; + async function initializeColumnAndTable(data: readonly SimpleTableRecord[], placeholder?: string): Promise { + column.placeholder = placeholder; + await table.setData(data); + await connect(); + await waitForUpdatesAsync(); + } + parameterizeSpec(testCases, (spec, name, value) => { spec( `cell and group row render expected value when ${name} and placeholder is configured`, async () => { const placeholder = 'Custom placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data, placeholder); const expectedCellText = value.usesColumnPlaceholder ? placeholder @@ -274,9 +278,7 @@ describe('TableColumnDurationText', () => { spec( `cell and group row render expected value when ${name} and placeholder is not configured`, async () => { - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data); expect(pageObject.getRenderedCellContent(0, 0)).toBe( value.cellValue @@ -290,39 +292,31 @@ describe('TableColumnDurationText', () => { it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { const placeholder = 'My placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder); expect(pageObject.getRenderedCellContent(0, 0)).toBe(placeholder); - elementReferences.column1.placeholder = undefined; + column.placeholder = undefined; await waitForUpdatesAsync(); expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); }); - it('setting placeholder to defined string updates cells from displaying placeholder to displaying blank', async () => { - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + it('setting placeholder to defined string updates cells from displaying blank to displaying placeholder', async () => { + await initializeColumnAndTable([{}]); expect(pageObject.getRenderedCellContent(0, 0)).toBe(''); const placeholder = 'placeholder'; - elementReferences.column1.placeholder = placeholder; + column.placeholder = placeholder; await waitForUpdatesAsync(); expect(pageObject.getRenderedCellContent(0, 0)).toBe(placeholder); }); it('updating placeholder from one string to another updates cell', async () => { const placeholder1 = 'My first placeholder'; - elementReferences.column1.placeholder = placeholder1; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder1); expect(pageObject.getRenderedCellContent(0, 0)).toBe(placeholder1); const placeholder2 = 'My second placeholder'; - elementReferences.column1.placeholder = placeholder2; + column.placeholder = placeholder2; await waitForUpdatesAsync(); expect(pageObject.getRenderedCellContent(0, 0)).toBe(placeholder2); }); diff --git a/packages/nimble-components/src/table-column/enum-text/tests/table-column-enum-text.mdx b/packages/nimble-components/src/table-column/enum-text/tests/table-column-enum-text.mdx index c28159afe9..2b7fdd6d19 100644 --- a/packages/nimble-components/src/table-column/enum-text/tests/table-column-enum-text.mdx +++ b/packages/nimble-components/src/table-column/enum-text/tests/table-column-enum-text.mdx @@ -19,8 +19,7 @@ The column renders string, number, or boole ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. -- Avoid using values that do not correspond to a mapping for the column. +- Provide a mapping for every expected record value. Because grouping is performed on the record value, non-mapped record values may be grouped unexpectedly and will not have a group label. ### Blazor Usage diff --git a/packages/nimble-components/src/table-column/icon/tests/table-column-icon.mdx b/packages/nimble-components/src/table-column/icon/tests/table-column-icon.mdx index cbc4d70a29..51e1a8e208 100644 --- a/packages/nimble-components/src/table-column/icon/tests/table-column-icon.mdx +++ b/packages/nimble-components/src/table-column/icon/tests/table-column-icon.mdx @@ -21,9 +21,8 @@ The column renders string, number, or boolean v ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. -- Avoid using values that do not correspond to a mapping for the column. -- To display an empty cell but have a non-blank group row, create a mapping of the record value to an `undefined` icon. +- Provide a mapping for every expected record value. Because grouping is performed on the record value, non-mapped record values may be grouped unexpectedly and will not have a group label. + - To improve grouping behavior of values that don't correspond to icons, explicitly map those values to an `undefined` icon. ### Blazor Usage diff --git a/packages/nimble-components/src/table-column/mixins/placeholder.ts b/packages/nimble-components/src/table-column/mixins/placeholder.ts index c3bb3c11e5..65844ccd4a 100644 --- a/packages/nimble-components/src/table-column/mixins/placeholder.ts +++ b/packages/nimble-components/src/table-column/mixins/placeholder.ts @@ -8,7 +8,7 @@ type TableColumnWithPlaceholder = Pick; // eslint-disable-next-line @typescript-eslint/no-explicit-any type TableColumnWithPlaceholderConstructor = abstract new (...args: any[]) => TableColumnWithPlaceholder; -// As the returned class is internal to the function, we can't write a signature that uses is directly, so rely on inference +// As the returned class is internal to the function, we can't write a signature that uses it directly, so rely on inference // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/explicit-function-return-type export function mixinColumnWithPlaceholderAPI< TBase extends TableColumnWithPlaceholderConstructor diff --git a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text-matrix.stories.ts b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text-matrix.stories.ts index de9c0f9564..a5c7e08f55 100644 --- a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text-matrix.stories.ts +++ b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text-matrix.stories.ts @@ -13,6 +13,7 @@ import { controlLabelFontColor } from '../../../theme-provider/design-tokens'; import { NumberTextAlignment } from '../types'; +import { placeholderStates, type PlaceholderState } from '../../../utilities/tests/states'; const metadata: Meta = { title: 'Tests/Table Column: Number Text', @@ -42,19 +43,13 @@ const alignmentStates: [string, string | undefined][] = Object.entries( ).map(([key, value]) => [pascalCase(key), value]); type AlignmentState = (typeof alignmentStates)[number]; -const placeholderStates: [string, string | undefined][] = [ - ['With Placeholder', 'Custom placeholder'], - ['', undefined] -]; -type PlaceholderState = (typeof placeholderStates)[number]; - // prettier-ignore const component = ( [alignmentName, alignment]: AlignmentState, [placeholderName, placeholder]: PlaceholderState ): ViewTemplate => html` <${tableTag} id-field-name="id" style="height: 450px"> <${tableColumnNumberTextTag} diff --git a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.mdx b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.mdx index 41ae26c53a..5ca9ed907f 100644 --- a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.mdx +++ b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.mdx @@ -1,6 +1,6 @@ import { Canvas, Meta, Controls, Title } from '@storybook/blocks'; import { NimbleTableColumnNumberText } from './table-column-number-text.react'; -import { columnOperationBehavior } from '../../base/tests/table-column-stories-utils'; +import { columnOperationBehavior, noNullAndUndefinedBestPractice } from '../../base/tests/table-column-stories-utils'; import * as tableColumnNumberTextStories from './table-column-number-text.stories'; import { tableColumnNumberTextTag } from '..'; import { tableTag } from '../../../table'; @@ -24,7 +24,7 @@ based on the value of the `lang` token, which can be set via the [nimble-theme-p ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. +- {noNullAndUndefinedBestPractice} - If relevant to your data source, make sure to consider the IEEE 754 special cases of `-Inf`, `+Inf`, `-0`, `+0`, and `NaN`. ### Angular Usage diff --git a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.spec.ts b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.spec.ts index 78f406b191..1d9db3e9ac 100644 --- a/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.spec.ts +++ b/packages/nimble-components/src/table-column/number-text/tests/table-column-number-text.spec.ts @@ -768,15 +768,19 @@ describe('TableColumnNumberText', () => { } ]; + async function initializeColumnAndTable(data: readonly SimpleTableRecord[], placeholder?: string): Promise { + elementReferences.column1.placeholder = placeholder; + await table.setData(data); + await connect(); + await waitForUpdatesAsync(); + } + parameterizeSpec(testCases, (spec, name, value) => { spec( `cell and group row render expected value when ${name} and placeholder is configured`, async () => { const placeholder = 'Custom placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data, placeholder); const expectedCellText = value.usesColumnPlaceholder ? placeholder @@ -795,9 +799,7 @@ describe('TableColumnNumberText', () => { spec( `cell and group row render expected value when ${name} and placeholder is not configured`, async () => { - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( value.cellValue @@ -811,10 +813,7 @@ describe('TableColumnNumberText', () => { it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { const placeholder = 'My placeholder'; - elementReferences.column1.placeholder = placeholder; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder ); @@ -824,10 +823,8 @@ describe('TableColumnNumberText', () => { expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); }); - it('setting placeholder to defined string updates cells from displaying placeholder to displaying blank', async () => { - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + it('setting placeholder to defined string updates cells from displaying blank to displaying placeholder', async () => { + await initializeColumnAndTable([{}]); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); const placeholder = 'placeholder'; @@ -840,10 +837,7 @@ describe('TableColumnNumberText', () => { it('updating placeholder from one string to another updates cell', async () => { const placeholder1 = 'My first placeholder'; - elementReferences.column1.placeholder = placeholder1; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder1); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder1 ); diff --git a/packages/nimble-components/src/table-column/text/tests/table-column-text-matrix.stories.ts b/packages/nimble-components/src/table-column/text/tests/table-column-text-matrix.stories.ts index 029e06417d..a8df411696 100644 --- a/packages/nimble-components/src/table-column/text/tests/table-column-text-matrix.stories.ts +++ b/packages/nimble-components/src/table-column/text/tests/table-column-text-matrix.stories.ts @@ -11,6 +11,7 @@ import { controlLabelFont, controlLabelFontColor } from '../../../theme-provider/design-tokens'; +import { placeholderStates, type PlaceholderState } from '../../../utilities/tests/states'; const metadata: Meta = { title: 'Tests/Table Column: Text', @@ -40,19 +41,13 @@ const data = [ } ] as const; -const placeholderStates: [string, string | undefined][] = [ - ['With Placeholder', 'Custom placeholder'], - ['', undefined] -]; -type PlaceholderState = (typeof placeholderStates)[number]; - // prettier-ignore const component = ( [placeholderName, placeholder]: PlaceholderState ): ViewTemplate => html` - + <${tableTag} id-field-name="id" style="height: 320px"> <${tableColumnTextTag} field-name="id" diff --git a/packages/nimble-components/src/table-column/text/tests/table-column-text.mdx b/packages/nimble-components/src/table-column/text/tests/table-column-text.mdx index 8ca6a7d0c2..224e398a2f 100644 --- a/packages/nimble-components/src/table-column/text/tests/table-column-text.mdx +++ b/packages/nimble-components/src/table-column/text/tests/table-column-text.mdx @@ -3,6 +3,7 @@ import { NimbleTableColumnText } from './table-column-text.react'; import * as tableColumnTextStories from './table-column-text.stories'; import { tableColumnTextTag } from '..'; import { tableTag } from '../../../table'; +import { noNullAndUndefinedBestPractice } from '../../base/tests/table-column-stories-utils'; @@ -20,7 +21,7 @@ The <Tag name={tableColumnTextTag}/> column is used to display string fields as ### Best Practices -- Avoid mixing `undefined` and `null` as values for the same field. When grouping this will lead to two groups (one for `null` values and one for `undefined` values) that both have the text `"No value"`. +- {noNullAndUndefinedBestPractice} - Avoid mixing empty string with `undefined`/`null`. The distinction when grouping between `"No value"` and `"Empty"` is not likely meaningful to a user. - Avoid displaying whitespace values that are not empty string (`''`) as these values will be rendered as-is in group rows. diff --git a/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts b/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts index 4685b61f0e..d0b0480fab 100644 --- a/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts +++ b/packages/nimble-components/src/table-column/text/tests/table-column-text.spec.ts @@ -249,18 +249,29 @@ describe('TableColumnText', () => { cellValue: '', groupValue: 'Empty', usesColumnPlaceholder: false + }, + { + name: 'value is a string containing only whitespace', + data: [{ field: ' ' }], + cellValue: '', + groupValue: '', + usesColumnPlaceholder: false } ]; + async function initializeColumnAndTable(data: readonly SimpleTableRecord[], placeholder?: string): Promise<void> { + column.placeholder = placeholder; + await table.setData(data); + await connect(); + await waitForUpdatesAsync(); + } + parameterizeSpec(testCases, (spec, name, value) => { spec( `cell and group row render expected value when ${name} and placeholder is configured`, async () => { const placeholder = 'Custom placeholder'; - column.placeholder = placeholder; - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data, placeholder); const expectedCellText = value.usesColumnPlaceholder ? placeholder @@ -279,9 +290,7 @@ describe('TableColumnText', () => { spec( `cell and group row render expected value when ${name} and placeholder is not configured`, async () => { - await table.setData(value.data); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable(value.data); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( value.cellValue @@ -295,10 +304,7 @@ describe('TableColumnText', () => { it('setting placeholder to undefined updates cells from displaying placeholder to displaying blank', async () => { const placeholder = 'My placeholder'; - column.placeholder = placeholder; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder ); @@ -308,10 +314,8 @@ describe('TableColumnText', () => { expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); }); - it('setting placeholder to defined string updates cells from displaying placeholder to displaying blank', async () => { - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + it('setting placeholder to defined string updates cells from displaying blank to displaying placeholder', async () => { + await initializeColumnAndTable([{}]); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe(''); const placeholder = 'placeholder'; @@ -324,10 +328,7 @@ describe('TableColumnText', () => { it('updating placeholder from one string to another updates cell', async () => { const placeholder1 = 'My first placeholder'; - column.placeholder = placeholder1; - await table.setData([{}]); - await connect(); - await waitForUpdatesAsync(); + await initializeColumnAndTable([{}], placeholder1); expect(pageObject.getRenderedCellTextContent(0, 0)).toBe( placeholder1 ); diff --git a/packages/nimble-components/src/utilities/tests/states.ts b/packages/nimble-components/src/utilities/tests/states.ts index a5b88d786c..bbe9820e11 100644 --- a/packages/nimble-components/src/utilities/tests/states.ts +++ b/packages/nimble-components/src/utilities/tests/states.ts @@ -41,3 +41,9 @@ export type ReadOnlyState = (typeof readOnlyStates)[number]; export const iconVisibleStates = [false, true] as const; export type IconVisibleState = (typeof iconVisibleStates)[number]; + +export const placeholderStates = [ + ['With Placeholder', 'Custom placeholder'], + ['', undefined] +] as const; +export type PlaceholderState = (typeof placeholderStates)[number];