Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0157f8f
implement withFluentVRTestVariants decorator
TristanWatanabe Oct 8, 2022
d00208f
update test to include new decorator
TristanWatanabe Oct 8, 2022
c797198
update typings
TristanWatanabe Oct 10, 2022
bda9ea4
fix: typing and update API
TristanWatanabe Oct 10, 2022
dcfcde3
Add usage of decorator to README
TristanWatanabe Oct 10, 2022
794bd7f
remove unneed peerDep
TristanWatanabe Oct 10, 2022
f306846
fix: typing
TristanWatanabe Oct 10, 2022
fb87708
Update API
TristanWatanabe Oct 10, 2022
7bf1873
revert additions to react-storybook package
TristanWatanabe Oct 12, 2022
ed168ee
delete withFluentVrTestVariants
TristanWatanabe Oct 12, 2022
d3765ac
remove withFluentVrTestVariant
TristanWatanabe Oct 12, 2022
1f26987
feat: withFluentProvider now accepts fluentTheme and dir
TristanWatanabe Oct 12, 2022
66c95ef
Add exported theme constants
TristanWatanabe Oct 12, 2022
ab22cd5
feat: add isVrTest parameter
TristanWatanabe Oct 13, 2022
060d696
Revert "Add usage of decorator to README"
TristanWatanabe Oct 13, 2022
18c3e6d
update ReadMe
TristanWatanabe Oct 13, 2022
14847d9
Add constants and FluentParameters to exports
TristanWatanabe Oct 13, 2022
20091d7
Update API
TristanWatanabe Oct 13, 2022
e840ba7
Revert "Update API"
TristanWatanabe Oct 13, 2022
21eba2e
Revert "fix: typing and update API"
TristanWatanabe Oct 13, 2022
81de523
feedback: don't export constants, export FluentParameter type
TristanWatanabe Oct 16, 2022
b5f3788
feedback: replace isVrTest param with more dynamic mode param
TristanWatanabe Oct 16, 2022
4842eb9
feedback: add parameters identity function and export
TristanWatanabe Oct 16, 2022
8317329
Update API
TristanWatanabe Oct 16, 2022
55836b6
Update README
TristanWatanabe Oct 16, 2022
1559515
Merge branch 'master' into add-VrTestVariants-decorator
TristanWatanabe Oct 16, 2022
d888f92
remove unintended prettier changes
TristanWatanabe Oct 20, 2022
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
29 changes: 27 additions & 2 deletions packages/react-components/react-storybook/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,33 @@ You need to register fluentui decorators on your particular level (global/story/
// @filename: .storybook/preview.js

import { withKnobs } from '@storybook/addon-knobs';
import { withStrictMode } from '@fluentui/react-storybook';
import { withStrictMode, withFluentVrTestVariants } from '@fluentui/react-storybook';

// Register decorators on global level
export const decorators = [withKnobs, withStrictMode];
export const decorators = [withKnobs, withStrictMode, withFluentVrTestVariants];
```

## withFluentVrTestVariants:

```js
import { withFluentVrTestVariants, DARK_MODE, HIGH_CONTRAST, RTL } from '@fluentui/react-storybook';
import { Button } from '@fluentui/react-components';

export const Button = () => <Button> Hello World </Button>;

export const ButtonDarkMode = {
render: Button,
parameters: { variant: DARK_MODE }, // story renders in Dark mode.
};

export const ButtonHighContrast = {
render: Button,
parameters: { variant: HIGH_CONTRAST }; // story renders in High Contrast mode.
}

export const ButtonRTL = {
render: Button,
parameters: { variant: RTL }, // story renders in RTL.
};

```
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,27 @@
> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).

```ts

import * as React_2 from 'react';

// @public (undocumented)
export const DARK_MODE = "DarkMode";

// @public (undocumented)
export const HIGH_CONTRAST = "HighContrast";

// @public (undocumented)
export const RTL = "Rtl";

// @public (undocumented)
export const withFluentProvider: (...args: any) => any;

// @public (undocumented)
export const withFluentVrTestVariants: () => React_2.ReactNode;

// @public (undocumented)
export const withStrictMode: (storyFn: () => React_2.ReactNode) => JSX.Element;

// (No @packageDocumentation comment for this package)

```
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './withFluentProvider';
export * from './withStrictMode';
export * from './withFluentVrTestVariants';
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import * as React from 'react';
import { FluentProvider } from '@fluentui/react-provider';
import { teamsHighContrastTheme, webDarkTheme, webLightTheme } from '@fluentui/react-theme';
import { makeDecorator } from '@storybook/addons';
import type { StoryContext } from '@storybook/addons';

export const DARK_MODE = 'DarkMode';
export const HIGH_CONTRAST = 'HighContrast';
export const RTL = 'Rtl';

// MakeDecoratorOptions is not an exported interface by storybook so had to include necessary options
interface AssertedMakeDecoratorOptions {
name: string;
parameterName: string;
skipIfNoParametersOrOptions?: boolean;
wrapper: (
storyFn: (context: StoryContext) => React.ReactNode,
context: StoryContext,
settings: {
parameters: typeof DARK_MODE | typeof HIGH_CONTRAST | typeof RTL;
},
) => React.ReactNode;
}

type AssertedMakeDecorator = ({
name,
parameterName,
skipIfNoParametersOrOptions,
wrapper,
}: AssertedMakeDecoratorOptions) => () => React.ReactNode;

export const withFluentVrTestVariants = ((makeDecorator as unknown) as AssertedMakeDecorator)({
name: 'withFluentVrTestVariants.',
parameterName: 'vrTestVariant',
skipIfNoParametersOrOptions: true,
wrapper: (storyFn, context, { parameters }) => {
if (parameters === RTL) {
return (
<FluentProvider dir={'rtl'} theme={webLightTheme}>
{storyFn(context)}
</FluentProvider>
);
}

if (parameters === DARK_MODE) {
return <FluentProvider theme={webDarkTheme}>{storyFn(context)}</FluentProvider>;
}

if (parameters === HIGH_CONTRAST) {
return <FluentProvider theme={teamsHighContrastTheme}>{storyFn(context)}</FluentProvider>;
}

return storyFn(context);
Copy link
Contributor

@ling1726 ling1726 Oct 11, 2022

Choose a reason for hiding this comment

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

It seems that we have this new decorator that configures the provider but also withFluentProvider that is not configurable. Then let's just have one withFluentProvider that is configurable ?

Copy link
Contributor

Choose a reason for hiding this comment

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

^ great call, lets do it ! #DRY

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I considered doing this but as it stands, the withFluentProvider decorator doesn’t just wrap a story function in FluentProvider, but it also wraps it in a container with custom styling. I wanted to avoid a bunch of Screener change churn that this would cause which is why I opted for a separate decorator.

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case we could remove the custom styling into its own decorator ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to avoid a bunch of Screener change churn

wondering what churn are you referring to ?

Copy link
Member Author

@TristanWatanabe TristanWatanabe Oct 12, 2022

Choose a reason for hiding this comment

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

Since an extra div with styling is essentially added to each story by the withFluentProvider decorator, when each story is converted to CSF , Screener will detect each one as "Changed". I was trying to avoid a bunch of unnecessary Screener changes when I refactor to CSF to simplify the review process and to not introduce random screener issues

Copy link
Member Author

Choose a reason for hiding this comment

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

in that case we could remove the custom styling into its own decorator ?

With the way it's implemented right now, that would just seem redundant? Even if moved as a separate decorator, it would still need a FluentProvider regardless.

export const withFluentProvider = (StoryFn: () => JSX.Element, context: FluentStoryContext) => {
  const { theme } = getActiveFluentTheme(context.globals);

  return (
    <FluentProvider theme={theme}>
      <FluentExampleContainer theme={theme}>{StoryFn()}</FluentExampleContainer>
    </FluentProvider>
  );
};

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not opposed to just reconfiguring the withFluentProvider so that it can accept theme and dir parameters and any screener changes (it adds padding and theme background) can just be communicated in the PR details - thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could utilize the storybook context or globals a bit more, withFluentProvider could maybe have like mode configuration where the VR tests would configure a mode that would not inject padding and theme background ?

},
});
4 changes: 2 additions & 2 deletions packages/react-components/react-storybook/src/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { withFluentProvider, withStrictMode } from './index';
import { withFluentProvider, withFluentVrTestVariants, withStrictMode } from './index';

describe(`public api`, () => {
describe(`decorators`, () => {
it(`should work`, () => {
const decorators = [withFluentProvider, withStrictMode];
const decorators = [withFluentProvider, withStrictMode, withFluentVrTestVariants];

// @TODO - added proper tests
expect(decorators).toBeDefined();
Expand Down
9 changes: 8 additions & 1 deletion packages/react-components/react-storybook/src/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,8 @@
export { withFluentProvider, withStrictMode } from './decorators/index';
export {
withFluentProvider,
withStrictMode,
withFluentVrTestVariants,
DARK_MODE,
HIGH_CONTRAST,
RTL,
} from './decorators/index';