PR feedback

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,

packages/nimble-components/src/table-column/anchor/tests/table-column-anchor.mdx

@@ -16,18 +16,11 @@ The <Tag name={tableColumnAnchorTag}/> 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
 

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<Fixture<Table<SimpleTableRecord>>> {
     return fixture<Table<SimpleTableRecord>>(
-        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}>
-                    <${tableColumnAnchorTag}>
-                        Column 2
-                    </${tableColumnAnchorTag}>
-                </${tableTag}>
-            </${themeProviderTag}>`,
+        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}>
+                <${tableColumnAnchorTag}>
+                    Column 2
+                </${tableColumnAnchorTag}>
+            </${tableTag}>`,
         { 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<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
@@ -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
             );

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"`.'

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

packages/nimble-components/src/table-column/date-text/tests/table-column-date-text.mdx

@@ -20,7 +20,7 @@ The <Tag name={tableColumnDateTextTag}/> 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 */}