From 1138c397bd66af309f16f12bdec3fb432d1b80b7 Mon Sep 17 00:00:00 2001 From: Sean Monahan Date: Wed, 2 Mar 2022 15:07:06 -0800 Subject: [PATCH 001/173] Add conformance tests for static classnames This commit adds conformance tests for static class names. The tests: 1. Ensure that the static classnames object is exported 2. Ensure that the static classnames object is formatted correctly 3. Ensure that the static classnames are rendered in the component The rules for static classnames are defined in RFC: Conventions for static class names on slots (#21206). Only Radio and RadioGroup have been updated to pass these test. --- .../src/defaultErrorMessages.tsx | 96 +++++++++++++- .../react-conformance/src/defaultTests.tsx | 117 ++++++++++++++++++ packages/react-conformance/src/types.ts | 3 + packages/react-radio/etc/react-radio.api.md | 5 +- .../src/components/Radio/Radio.test.tsx | 7 ++ .../src/components/Radio/useRadioStyles.ts | 45 ++++--- .../RadioGroup/useRadioGroupStyles.ts | 8 +- 7 files changed, 256 insertions(+), 25 deletions(-) diff --git a/packages/react-conformance/src/defaultErrorMessages.tsx b/packages/react-conformance/src/defaultErrorMessages.tsx index eadfcf7f8c842c..d0016f97ce2219 100644 --- a/packages/react-conformance/src/defaultErrorMessages.tsx +++ b/packages/react-conformance/src/defaultErrorMessages.tsx @@ -510,7 +510,7 @@ export const defaultErrorMessages = { indexFile, )}.`, suggestions: [ - `Make sure that your component's ${resolveInfo('index.ts')} file` + + `Make sure that your component's ${resolveInfo('index.ts')} file ` + `or a file with styles hook exports \`${resolveInfo(constantValue)}\``, `If the component is internal, consider enabling ${resolveInfo('isInternal')} in your isConformant test.`, ], @@ -518,6 +518,100 @@ export const defaultErrorMessages = { }); }, + 'component-has-static-classnames-object-exported': ( + testInfo: IsConformantOptions, + error: Error, + componentClassName: string, + exportName: string, + ) => { + const { componentPath, displayName } = testInfo; + const { testErrorInfo, resolveInfo, testErrorPath } = errorMessageColors; + const indexFile = path.join(getPackagePath(componentPath), 'src', 'index.ts'); + + return getErrorMessage({ + displayName, + overview: `static classNames object is not exported (${testErrorInfo(exportName)}) in: ${EOL}${testErrorPath( + indexFile, + )}.`, + suggestions: [ + `Make sure that your component's ${resolveInfo('index.ts')} file ` + + `or a file with styles hook exports an object of type SlotClassNames.`, + `If the component is internal, consider enabling ${resolveInfo('isInternal')} in your isConformant test.`, + ], + error, + }); + }, + + 'component-has-static-classnames-in-correct-format': ( + testInfo: IsConformantOptions, + error: Error, + componentClassName: string, + exportName: string, + ) => { + const { componentPath, displayName } = testInfo; + const { testErrorInfo, resolveInfo, testErrorPath } = errorMessageColors; + const indexFile = path.join(getPackagePath(componentPath), 'src', 'index.ts'); + + return getErrorMessage({ + displayName, + overview: `static classNames object is not formatted correctly (${testErrorInfo( + exportName, + )}) in: ${EOL}${testErrorPath(indexFile)}.`, + suggestions: [ + `Make sure all classNames are for the form "fui-__" ` + + `or "fui-" for the root slot`, + `If the component is internal, consider enabling ${resolveInfo('isInternal')} in your isConformant test.`, + ], + error, + }); + }, + + 'component-has-static-classnames': ( + testInfo: IsConformantOptions, + error: Error, + componentName: string, + missingClassNames: string, + ) => { + const { displayName } = testInfo; + const { testErrorInfo, failedError } = errorMessageColors; + + return getErrorMessage({ + displayName, + overview: `missing one or more static classNames on component (${testErrorInfo(componentName)}).`, + details: [`Missing the following classes after render:`, ` ${failedError(missingClassNames)}`], + suggestions: [`Ensure that each slot of the component has its corresponding static className.`], + error, + }); + }, + + 'as-renders-html': (testInfo: IsConformantOptions, error: Error) => { + const { displayName } = testInfo; + const { resolveInfo } = errorMessageColors; + + // Message Description: Receives an error when attempting to render as a functional component + // or pass as to the next component. + // + // It appears that "displayName" "as" prop doesn't properly handle HTML tags. + // Possible solutions: + // - If your component doesn't have an "as" prop, enable isConformant's skipAsPropTests option. + // - Make sure that your component can correctly render as HTML tags. + // - Check if you are missing any requiredProps within the isConformant in your test file. + // - Make sure that your component's implementation contains a valid return statement. + // - Check to see if your component works as expected with Enzyme's mount(). + return getErrorMessage({ + displayName, + overview: `"as" prop doesn't properly handle HTML tags.`, + suggestions: [ + `If your component doesn't have an "as" prop, enable isConformant's ${resolveInfo('skipAsPropTests')} option.`, + `Make sure that your component can correctly render as ${resolveInfo('HTML tags')}.`, + `Check if you are missing any ${resolveInfo('requiredProps')} within the isConformant in your test file.`, + `Make sure that your component's implementation contains a valid return statement.`, + `Check to see if your component works as expected with Enzyme's ${resolveInfo('mount()')}.`, + ], + error, + }); + }, + 'primary-slot-gets-native-props': (testInfo: IsConformantOptions, error: Error) => { const { displayName } = testInfo; const { resolveInfo } = errorMessageColors; diff --git a/packages/react-conformance/src/defaultTests.tsx b/packages/react-conformance/src/defaultTests.tsx index 890942500123aa..906bf1b346bf63 100644 --- a/packages/react-conformance/src/defaultTests.tsx +++ b/packages/react-conformance/src/defaultTests.tsx @@ -302,6 +302,123 @@ export const defaultTests: TestObject = { }); }, + 'component-has-static-classnames-object': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => { + const { + componentPath, + Component, + wrapperComponent, + helperComponents = [], + requiredProps, + customMount = mount, + } = testInfo; + + const componentName = componentInfo.displayName; + const componentClassName = `fui-${componentName}`; + let handledClassNamesObjectExport = false; + + it('has static classnames exported at top-level (component-has-static-classnames-object)', () => { + if (testInfo.isInternal) { + return; + } + + const exportName = + componentInfo.displayName.slice(0, 1).toLowerCase() + componentInfo.displayName.slice(1) + 'ClassNames'; + + try { + const indexFile = require(path.join(getPackagePath(componentPath), 'src', 'index')); + const classNamesFromFile = indexFile[exportName]; + expect(classNamesFromFile).toBeTruthy(); + handledClassNamesObjectExport = true; + } catch (e) { + // Need to work on this error message + throw new Error( + defaultErrorMessages['component-has-static-classnames-object-exported']( + testInfo, + e, + componentClassName, + exportName, + ), + ); + } + }); + + it('has static classnames in correct format (component-has-static-classnames-object)', () => { + if (!handledClassNamesObjectExport) { + return; + } + + const exportName = + componentInfo.displayName.slice(0, 1).toLowerCase() + componentInfo.displayName.slice(1) + 'ClassNames'; + const indexFile = require(path.join(getPackagePath(componentPath), 'src', 'index')); + const classNamesFromFile = indexFile[exportName]; + + const expectedClassNames = Object.keys(classNamesFromFile).reduce( + (obj: { [key: string]: string }, key: string) => { + obj[key] = key === 'root' ? componentClassName : `${componentClassName}__${key}`; + return obj; + }, + {}, + ); + + try { + expect(classNamesFromFile).toEqual(expectedClassNames); + } catch (e) { + // Need to work on this error message + throw new Error( + defaultErrorMessages['component-has-static-classnames-in-correct-format']( + testInfo, + e, + componentClassName, + exportName, + ), + ); + } + }); + + it(`has static classnames (component-has-static-classnames-object)`, () => { + if (!handledClassNamesObjectExport) { + return; + } + + const { testOptions = {} } = testInfo; + const mergedProps = { + ...requiredProps, + ...(testOptions['has-static-classnames'] ?? {}), + }; + const defaultEl = customMount(); + const defaultComponent = getComponent(defaultEl, helperComponents, wrapperComponent); + + const exportName = + componentInfo.displayName.slice(0, 1).toLowerCase() + componentInfo.displayName.slice(1) + 'ClassNames'; + const indexFile = require(path.join(getPackagePath(componentPath), 'src', 'index')); + const classNamesFromFile = indexFile[exportName]; + + const missingClassNames = []; + let className; + let err: Error; + for (const key of Object.keys(classNamesFromFile)) { + className = classNamesFromFile[key]; + try { + expect(defaultComponent.find(`.${className}`).length).toBeGreaterThanOrEqual(1); + } catch (e) { + err = e as Error; + missingClassNames.push(className); + } + } + + if (missingClassNames.length > 0) { + throw new Error( + defaultErrorMessages['component-has-static-classnames']( + testInfo, + err!, + componentName, + missingClassNames.join(', '), + ), + ); + } + }); + }, + /** Constructor/component name matches filename */ 'name-matches-filename': (componentInfo: ComponentDoc, testInfo: IsConformantOptions) => { it(`Component/constructor name matches filename (name-matches-filename)`, () => { diff --git a/packages/react-conformance/src/types.ts b/packages/react-conformance/src/types.ts index 31eba78daa349e..0cbbe54ce249ff 100644 --- a/packages/react-conformance/src/types.ts +++ b/packages/react-conformance/src/types.ts @@ -14,6 +14,9 @@ export interface TestOptions { 'consistent-callback-args'?: { ignoreProps?: string[]; }; + 'has-static-classnames'?: { + [key: string]: string; + }; } export interface IsConformantOptions { diff --git a/packages/react-radio/etc/react-radio.api.md b/packages/react-radio/etc/react-radio.api.md index 6fec70ebb1e8a3..e4f6825ac2372b 100644 --- a/packages/react-radio/etc/react-radio.api.md +++ b/packages/react-radio/etc/react-radio.api.md @@ -10,18 +10,19 @@ import type { ForwardRefComponent } from '@fluentui/react-utilities'; import { Label } from '@fluentui/react-label'; import * as React_2 from 'react'; import type { Slot } from '@fluentui/react-utilities'; +import type { SlotClassNames } from '@fluentui/react-utilities'; // @public export const Radio: ForwardRefComponent; // @public (undocumented) -export const radioClassName = "fui-Radio"; +export const radioClassNames: SlotClassNames; // @public export const RadioGroup: ForwardRefComponent; // @public (undocumented) -export const radioGroupClassName = "fui-RadioGroup"; +export const radioGroupClassNames: SlotClassNames; // @public export const RadioGroupContext: React_2.Context; diff --git a/packages/react-radio/src/components/Radio/Radio.test.tsx b/packages/react-radio/src/components/Radio/Radio.test.tsx index 1bb76f3151bd14..9a75446c205a33 100644 --- a/packages/react-radio/src/components/Radio/Radio.test.tsx +++ b/packages/react-radio/src/components/Radio/Radio.test.tsx @@ -8,6 +8,13 @@ describe('Radio', () => { Component: Radio, displayName: 'Radio', primarySlot: 'input', + testOptions: { + 'has-static-classnames': { + label: 'Test Label', + }, + // TODO: https://github.com/microsoft/fluentui/issues/19618 + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any, }); it('renders a default state', () => { diff --git a/packages/react-radio/src/components/Radio/useRadioStyles.ts b/packages/react-radio/src/components/Radio/useRadioStyles.ts index 0895d266b901ad..d2eaf8bd8c2158 100644 --- a/packages/react-radio/src/components/Radio/useRadioStyles.ts +++ b/packages/react-radio/src/components/Radio/useRadioStyles.ts @@ -1,11 +1,16 @@ import { createFocusOutlineStyle } from '@fluentui/react-tabster'; import { tokens } from '@fluentui/react-theme'; import { makeStyles, mergeClasses, shorthands } from '@griffel/react'; -import type { RadioState } from './Radio.types'; +import type { RadioSlots, RadioState } from './Radio.types'; +import type { SlotClassNames } from '@fluentui/react-utilities'; export const radioClassName = 'fui-Radio'; -const indicatorClassName = 'fui-Radio__indicator'; -const labelClassName = 'fui-Radio__label'; +export const radioClassNames: SlotClassNames = { + root: 'fui-Radio', + indicator: 'fui-Radio__indicator', + input: 'fui-Radio__input', + label: 'fui-Radio__label', +}; // TODO replace these spacing constants with theme values once they're on the theme const spacingHorizontalS = '8px'; @@ -50,33 +55,33 @@ const useInputStyles = makeStyles({ }, // When unchecked, hide the circle icon (child of the indicator) - [`:not(:checked) ~ .${indicatorClassName} > *`]: { + [`:not(:checked) ~ .${radioClassNames.indicator} > *`]: { opacity: '0', }, // Colors for the unchecked state ':enabled:not(:checked)': { - [`& ~ .${labelClassName}`]: { + [`& ~ .${radioClassNames.label}`]: { color: tokens.colorNeutralForeground3, }, - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorNeutralStrokeAccessible), }, ':hover': { - [`& ~ .${labelClassName}`]: { + [`& ~ .${radioClassNames.label}`]: { color: tokens.colorNeutralForeground2, }, - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorNeutralStrokeAccessibleHover), }, }, ':hover:active': { - [`& ~ .${labelClassName}`]: { + [`& ~ .${radioClassNames.label}`]: { color: tokens.colorNeutralForeground1, }, - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorNeutralStrokeAccessiblePressed), }, }, @@ -84,23 +89,23 @@ const useInputStyles = makeStyles({ // Colors for the checked state ':enabled:checked': { - [`& ~ .${labelClassName}`]: { + [`& ~ .${radioClassNames.label}`]: { color: tokens.colorNeutralForeground1, }, - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorCompoundBrandStroke), color: tokens.colorCompoundBrandForeground1, }, ':hover': { - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorCompoundBrandStrokeHover), color: tokens.colorCompoundBrandForeground1Hover, }, }, ':hover:active': { - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorCompoundBrandStrokePressed), color: tokens.colorCompoundBrandForeground1Pressed, }, @@ -109,10 +114,10 @@ const useInputStyles = makeStyles({ // Colors for the disabled state ':disabled': { - [`& ~ .${labelClassName}`]: { + [`& ~ .${radioClassNames.label}`]: { color: tokens.colorNeutralForegroundDisabled, }, - [`& ~ .${indicatorClassName}`]: { + [`& ~ .${radioClassNames.indicator}`]: { ...shorthands.borderColor(tokens.colorNeutralStrokeDisabled), color: tokens.colorNeutralForegroundDisabled, }, @@ -161,7 +166,7 @@ const useLabelStyles = makeStyles({ export const useRadioStyles_unstable = (state: RadioState) => { const rootStyles = useRootStyles(); state.root.className = mergeClasses( - radioClassName, + radioClassNames.root, rootStyles.base, rootStyles.focusIndicator, state.labelPosition === 'below' && rootStyles.vertical, @@ -169,15 +174,15 @@ export const useRadioStyles_unstable = (state: RadioState) => { ); const inputStyles = useInputStyles(); - state.input.className = mergeClasses(inputStyles.base, state.input.className); + state.input.className = mergeClasses(radioClassNames.input, inputStyles.base, state.input.className); const indicatorStyles = useIndicatorStyles(); - state.indicator.className = mergeClasses(indicatorClassName, indicatorStyles.base, state.indicator.className); + state.indicator.className = mergeClasses(radioClassNames.indicator, indicatorStyles.base, state.indicator.className); const labelStyles = useLabelStyles(); if (state.label) { state.label.className = mergeClasses( - labelClassName, + radioClassNames.label, labelStyles.base, state.labelPosition === 'below' && labelStyles.below, state.label.className, diff --git a/packages/react-radio/src/components/RadioGroup/useRadioGroupStyles.ts b/packages/react-radio/src/components/RadioGroup/useRadioGroupStyles.ts index eb21272f801384..8e11856677dd08 100644 --- a/packages/react-radio/src/components/RadioGroup/useRadioGroupStyles.ts +++ b/packages/react-radio/src/components/RadioGroup/useRadioGroupStyles.ts @@ -1,7 +1,11 @@ import { makeStyles, mergeClasses } from '@griffel/react'; -import { RadioGroupState } from './RadioGroup.types'; +import { RadioGroupSlots, RadioGroupState } from './RadioGroup.types'; +import type { SlotClassNames } from '@fluentui/react-utilities'; export const radioGroupClassName = 'fui-RadioGroup'; +export const radioGroupClassNames: SlotClassNames = { + root: 'fui-RadioGroup', +}; const useStyles = makeStyles({ root: { @@ -20,7 +24,7 @@ const useStyles = makeStyles({ export const useRadioGroupStyles_unstable = (state: RadioGroupState) => { const styles = useStyles(); state.root.className = mergeClasses( - radioGroupClassName, + radioGroupClassNames.root, styles.root, state.layout === 'vertical' && styles.vertical, state.root.className, From 4ce447b8003b6117ad8c14c85099948453b28a89 Mon Sep 17 00:00:00 2001 From: Sean Monahan Date: Fri, 4 Mar 2022 16:03:30 -0800 Subject: [PATCH 002/173] Update component creation template Adds support for static class names objects to the component creation template. See: https://github.com/microsoft/fluentui/issues/21847 --- .../{{componentName}}/use{{componentName}}Styles.ts.hbs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/create-component/plop-templates/src/components/{{componentName}}/use{{componentName}}Styles.ts.hbs b/scripts/create-component/plop-templates/src/components/{{componentName}}/use{{componentName}}Styles.ts.hbs index d15a6c40364bc0..6937d849e8eee2 100644 --- a/scripts/create-component/plop-templates/src/components/{{componentName}}/use{{componentName}}Styles.ts.hbs +++ b/scripts/create-component/plop-templates/src/components/{{componentName}}/use{{componentName}}Styles.ts.hbs @@ -1,8 +1,15 @@ import { makeStyles, mergeClasses } from '@griffel/react'; -import type { {{componentName}}State } from './{{componentName}}.types'; +import type { {{componentName}}Slots, {{componentName}}State } from './{{componentName}}.types'; +import type { SlotClassNames } from '@fluentui/react-utilities'; export const {{componentNames.propertyName}}ClassName = 'fui-{{componentName}}' +export const {{componentNames.propertyName}}ClassNames:SlotClassNames<{{componentName}}Slots> = { + root: 'fui-{{componentName}}' + // TODO: add class names for all slots on {{componentName}}Slots. + // Should be of the form `: 'fui-{{componentName}}__` +}; + /** * Styles for the root slot */ From aa272302b250ca0665514e51e9977f942c69dbea Mon Sep 17 00:00:00 2001 From: Sean Monahan Date: Fri, 4 Mar 2022 16:17:06 -0800 Subject: [PATCH 003/173] Add static classnames to Accordion Updates the react-accordion package to support static classnames. See: https://github.com/microsoft/fluentui/issues/21847 --- .../components/Accordion/useAccordionStyles.ts | 11 +++++++++-- .../AccordionHeader/AccordionHeader.test.tsx | 5 +++++ .../__snapshots__/AccordionHeader.test.tsx.snap | 4 ++-- .../AccordionHeader/useAccordionHeaderStyles.ts | 17 +++++++++++++++-- .../AccordionItem/useAccordionItemStyles.ts | 11 +++++++++-- .../AccordionPanel/useAccordionPanelStyles.ts | 11 +++++++++-- 6 files changed, 49 insertions(+), 10 deletions(-) diff --git a/packages/react-accordion/src/components/Accordion/useAccordionStyles.ts b/packages/react-accordion/src/components/Accordion/useAccordionStyles.ts index 10bb30b4281c91..6a5250e01fe520 100644 --- a/packages/react-accordion/src/components/Accordion/useAccordionStyles.ts +++ b/packages/react-accordion/src/components/Accordion/useAccordionStyles.ts @@ -1,10 +1,17 @@ +import { SlotClassNames } from '@fluentui/react-utilities/src'; import { mergeClasses } from '@griffel/react'; -import type { AccordionState } from './Accordion.types'; +import type { AccordionSlots, AccordionState } from './Accordion.types'; +/** + * @deprecated + */ export const accordionClassName = 'fui-Accordion'; +export const accordionClassNames: SlotClassNames = { + root: 'fui-Accordion', +}; export const useAccordionStyles_unstable = (state: AccordionState) => { - state.root.className = mergeClasses(accordionClassName, state.root.className); + state.root.className = mergeClasses(accordionClassNames.root, state.root.className); return state; }; diff --git a/packages/react-accordion/src/components/AccordionHeader/AccordionHeader.test.tsx b/packages/react-accordion/src/components/AccordionHeader/AccordionHeader.test.tsx index e9eec27b560fcb..63704ed39da3f7 100644 --- a/packages/react-accordion/src/components/AccordionHeader/AccordionHeader.test.tsx +++ b/packages/react-accordion/src/components/AccordionHeader/AccordionHeader.test.tsx @@ -14,6 +14,11 @@ describe('AccordionHeader', () => { Component: AccordionHeader, displayName: 'AccordionHeader', helperComponents: [AccordionHeaderContext.Provider], + testOptions: { + 'has-static-classnames': { + icon: 'Test Icon', + }, + }, }); afterEach(() => { diff --git a/packages/react-accordion/src/components/AccordionHeader/__snapshots__/AccordionHeader.test.tsx.snap b/packages/react-accordion/src/components/AccordionHeader/__snapshots__/AccordionHeader.test.tsx.snap index 8b902553c52158..d7d67797c40dd2 100644 --- a/packages/react-accordion/src/components/AccordionHeader/__snapshots__/AccordionHeader.test.tsx.snap +++ b/packages/react-accordion/src/components/AccordionHeader/__snapshots__/AccordionHeader.test.tsx.snap @@ -6,13 +6,13 @@ exports[`AccordionHeader renders a default state 1`] = ` >