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: redo FluentProvider theme api #21286

Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "make the FluentProvider#theme TS API reflect runtime and add warning if there is a violation",
"packageName": "@fluentui/react-provider",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "make the ThemeContext TS API reflect runtime and remove redundant ThemeContextValue",
"packageName": "@fluentui/react-shared-contexts",
"email": "[email protected]",
"dependentChangeType": "patch"
}
7 changes: 2 additions & 5 deletions packages/react-provider/etc/react-provider.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type { ProviderContextValue } from '@fluentui/react-shared-contexts';
import * as React_2 from 'react';
import type { Theme } from '@fluentui/react-theme';
import type { ThemeClassNameContextValue } from '@fluentui/react-shared-contexts';
import type { ThemeContextValue } from '@fluentui/react-shared-contexts';
import type { TooltipContextType } from '@fluentui/react-shared-contexts';
import { useFluent } from '@fluentui/react-shared-contexts';
import { useTheme } from '@fluentui/react-shared-contexts';
Expand All @@ -30,12 +29,10 @@ export interface FluentProviderCommons {
}

// @public (undocumented)
export interface FluentProviderContextValues {
export interface FluentProviderContextValues extends Pick<FluentProviderState, 'theme'> {
// (undocumented)
provider: ProviderContextValue;
// (undocumented)
theme: ThemeContextValue;
// (undocumented)
themeClassName: ThemeClassNameContextValue;
// (undocumented)
tooltip: TooltipContextType;
Expand All @@ -55,7 +52,7 @@ export type FluentProviderSlots = {
// @public (undocumented)
export interface FluentProviderState extends ComponentState<FluentProviderSlots>, FluentProviderCommons {
// (undocumented)
theme: Theme;
theme: Theme | Partial<Theme> | undefined;
// (undocumented)
themeClassName: string;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ import { isConformant } from '../../common/isConformant';
import { ProviderContext } from '@fluentui/react-shared-contexts';

describe('FluentProvider', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const noop = () => {};

beforeEach(() => {
jest.spyOn(console, 'warn').mockImplementation(noop);
});

isConformant({
disabledTests: ['component-handles-classname'],
Component: FluentProvider,
Expand All @@ -22,7 +29,9 @@ describe('FluentProvider', () => {
* Note: see more visual regression tests for FluentProvider in /apps/vr-tests.
*/
it('renders a default state', () => {
const component = renderer.create(<FluentProvider>Default FluentProvider</FluentProvider>);
const component = renderer.create(
<FluentProvider theme={{ colorBrandBackground2: '#fff' }}>Default FluentProvider</FluentProvider>,
);
const tree = component.toJSON();
expect(tree).toMatchSnapshot();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type { PartialTheme, Theme } from '@fluentui/react-theme';
import type {
ProviderContextValue,
TooltipContextType,
ThemeContextValue,
ThemeClassNameContextValue,
} from '@fluentui/react-shared-contexts';

Expand All @@ -26,13 +25,12 @@ export interface FluentProviderProps
}

export interface FluentProviderState extends ComponentState<FluentProviderSlots>, FluentProviderCommons {
theme: Theme;
theme: Theme | Partial<Theme> | undefined;
themeClassName: string;
}

export interface FluentProviderContextValues {
export interface FluentProviderContextValues extends Pick<FluentProviderState, 'theme'> {
provider: ProviderContextValue;
theme: ThemeContextValue;
themeClassName: ThemeClassNameContextValue;
tooltip: TooltipContextType;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,26 @@ import { useFluentProvider } from './useFluentProvider';
import type { PartialTheme } from '@fluentui/react-theme';

describe('useFluentProvider', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const noop = () => {};
let logWarnSpy: jest.SpyInstance;

beforeEach(() => {
logWarnSpy = jest.spyOn(console, 'warn').mockImplementation(noop);
});

it(`should warn user if no theme was set in parent or child`, () => {
const Wrapper: React.FC = ({ children }) => <FluentProvider>{children}</FluentProvider>;

const { result } = renderHook(() => useFluentProvider({}, React.createRef()), {
wrapper: Wrapper,
});

expect(result.current.theme).toBe(undefined);
expect(logWarnSpy).toHaveBeenCalledTimes(2);
expect(logWarnSpy).toHaveBeenCalledWith(expect.stringContaining('FluentProvider: your "theme" is not defined !'));
});

it('should merge themes', () => {
const themeA: PartialTheme = {
strokeWidthThick: '10px',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,21 @@ export const useFluentProvider = (props: FluentProviderProps, ref: React.Ref<HTM
* nesting providers with the same "dir" should not add additional attributes to DOM
* see https://github.com/microsoft/fluentui/blob/0dc74a19f3aa5a058224c20505016fbdb84db172/packages/fluentui/react-northstar/src/utils/mergeProviderContexts.ts#L89-L93
*/
const { dir = parentContext.dir, targetDocument = parentContext.targetDocument, theme = {} } = props;
const { dir = parentContext.dir, targetDocument = parentContext.targetDocument, theme } = props;
const mergedTheme = mergeThemes(parentTheme, theme);

React.useEffect(() => {
if (process.env.NODE_ENV !== 'production' && mergedTheme === undefined) {
Hotell marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line no-console
console.warn(`
FluentProvider: your "theme" is not defined !
=============================================
Make sure your root FluentProvider has set a theme or you're setting the theme in your child FluentProvider.
`);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return {
dir,
targetDocument,
Expand All @@ -45,7 +57,7 @@ export const useFluentProvider = (props: FluentProviderProps, ref: React.Ref<HTM
};
};

function mergeThemes(a: Theme | undefined, b: Partial<Theme> | Theme | undefined): Theme {
function mergeThemes(a: Theme | Partial<Theme> | undefined, b: typeof a): Theme | Partial<Theme> | undefined {
// Merge impacts perf: we should like to avoid it if it's possible
if (a && b) {
return { ...a, ...b };
Expand All @@ -55,5 +67,5 @@ function mergeThemes(a: Theme | undefined, b: Partial<Theme> | Theme | undefined
return a;
}

return b as Theme;
return b;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ import { useFluentProvider } from './useFluentProvider';
import { useFluentProviderContextValues } from './useFluentProviderContextValues';

describe('useFluentProviderContextValues', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const noop = () => {};

beforeEach(() => {
jest.spyOn(console, 'warn').mockImplementation(noop);
});

it('should return a value for "provider"', () => {
const { result } = renderHook(() => {
const state = useFluentProvider({}, React.createRef());
Expand All @@ -24,17 +31,27 @@ describe('useFluentProviderContextValues', () => {
return useFluentProviderContextValues(state);
});

expect(result.current.tooltip).toBeDefined();
expect(result.current.tooltip).toEqual({});
});

it('should return a value for "theme"', () => {
it('should return undefined if "theme" is not set', () => {
const { result } = renderHook(() => {
const state = useFluentProvider({}, React.createRef());

return useFluentProviderContextValues(state);
});

expect(result.current.theme).toBeDefined();
expect(result.current.theme).toBe(undefined);
});

it('should return a value for "theme"', () => {
const { result } = renderHook(() => {
const state = useFluentProvider({ theme: { colorBrandBackground: '#fff' } }, React.createRef());

return useFluentProviderContextValues(state);
});

expect(result.current.theme).toEqual({ colorBrandBackground: '#fff' });
});

it('should return a value for "themeClassname"', () => {
Expand All @@ -44,6 +61,6 @@ describe('useFluentProviderContextValues', () => {
return useFluentProviderContextValues(state);
});

expect(typeof result.current.themeClassName).toBe('string');
expect(result.current.themeClassName).toBe('foo');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ export const useThemeStyleTag = (options: Pick<FluentProviderState, 'theme' | 't
}, [styleTagId, targetDocument]);

const cssRule = React.useMemo(() => {
const cssVarsAsString = Object.keys(theme).reduce((cssVarRule, cssVar) => {
cssVarRule += `--${cssVar}: ${theme[cssVar as keyof typeof theme]}; `;
return cssVarRule;
}, '');
const cssVarsAsString = theme
? (Object.keys(theme) as (keyof typeof theme)[]).reduce((cssVarRule, cssVar) => {
cssVarRule += `--${cssVar}: ${theme[cssVar]}; `;
return cssVarRule;
}, '')
: '';

// result: .fluent-provider1 { --css-var: '#fff' }
return `.${styleTagId} { ${cssVarsAsString} }`;
Expand All @@ -48,8 +50,7 @@ export const useThemeStyleTag = (options: Pick<FluentProviderState, 'theme' | 't
React.useEffect(() => {
return () => {
if (styleTag) {
// IE11 safe node removal, otherwise use node.remove()
styleTag.parentElement?.removeChild(styleTag);
styleTag.remove();
}
};
}, [styleTag]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ export const ThemeClassNameContext: React_2.Context<string>;
export type ThemeClassNameContextValue = string;

// @public (undocumented)
export const ThemeContext: React_2.Context<ThemeContextValue>;

// @public (undocumented)
export interface ThemeContextValue extends Theme {
}
export const ThemeContext: React_2.Context<Theme | Partial<Theme> | undefined>;

// @public
export const TooltipContext: React_2.Context<TooltipContextType>;
Expand All @@ -56,7 +52,7 @@ export function useFluent(): ProviderContextValue;
export const useMenuContext: () => MinimalMenuProps;

// @public (undocumented)
export function useTheme(): ThemeContextValue;
export function useTheme(): Theme | Partial<Theme> | undefined;

// @public (undocumented)
export function useThemeClassName(): ThemeClassNameContextValue;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as React from 'react';
import type { ThemeContextValue } from './ThemeContext.types';
import type { Theme } from '@fluentui/react-theme';

export const ThemeContext = React.createContext((null as unknown) as ThemeContextValue);
export const ThemeContext = React.createContext<Theme | Partial<Theme> | undefined>(undefined);

export function useTheme(): ThemeContextValue {
export function useTheme(): Theme | Partial<Theme> | undefined {
return React.useContext(ThemeContext);
}

This file was deleted.

1 change: 0 additions & 1 deletion packages/react-shared-contexts/src/ThemeContext/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export * from './ThemeContext';
export * from './ThemeContext.types';