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

fix(react-storybook-addon): transform decorator to function in withAriaLive() #32011

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jul 16, 2024

Previous Behavior

#31794 added a new decorator withAriaLive(). That decorator has conditional rendering:

return <AriaLiveAnnouncer>{mounted && props.children}</AriaLiveAnnouncer>;

The problem is that it's implemented according to Storybook 6 docs and stories as a function:

return <AriaLiveWrapper>{StoryFn()}</AriaLiveWrapper>;

That causes an issue with a conditional rendering due effect order:

- useEffect: in story (as `withAriaLive()` call a function) [RENDER 1]
- useEffect: AriaLiveWrapper (mount) [RENDER 1]

This issue breaks stories that rely on useEffect(fn, []) as they are executed in wrong order.

New Behavior

The PR changes decorators to be called as React components. The same is suggested in Storybook 7 & 8 docs (https://storybook.js.org/docs/writing-stories/decorators#component-decorators). The PR updates withAriaLive() with an explicit condition for VR tests as VR tooling will fail to get stories for steps otherwise (see findSteps() in node_modules/storywright/src/StoryWrightProcessor/GetStories.js)

This fixed the order or effects:

- useEffect: AriaLiveWrapper (mount) [RENDER 1]
- useEffect: in story [RENDER 2]

Related Issue(s)

Fixes #32010.

@github-actions github-actions bot added this to the July Project Cycle Q3 2024 milestone Jul 16, 2024
@fabricteam
Copy link
Collaborator

fabricteam commented Jul 16, 2024

📊 Bundle size report

✅ No changes found

@layershifter layershifter marked this pull request as ready for review July 16, 2024 09:56
@layershifter layershifter requested a review from a team as a code owner July 16, 2024 09:56
@layershifter layershifter marked this pull request as draft July 16, 2024 09:59
@layershifter layershifter force-pushed the fix/decorators branch 4 times, most recently from b67556e to 1f2491c Compare July 16, 2024 13:39
@layershifter layershifter marked this pull request as ready for review July 16, 2024 13:45
@Hotell
Copy link
Contributor

Hotell commented Jul 16, 2024

can we postpone this until #31939 lands as there seems to be a lot of VR diffs caused by this change ?

@Hotell Hotell changed the title fix(storybook-addon): fix decorators to functions fix(react-storybook-addon): fix decorators to functions Jul 16, 2024
@layershifter layershifter changed the title fix(react-storybook-addon): fix decorators to functions fix(react-storybook-addon): transform decorator to function in withAriaLive() Jul 16, 2024
@layershifter
Copy link
Member Author

can we postpone this until #31939 lands as there seems to be a lot of VR diffs caused by this change ?

@Hotell It was a bug in my code, now there are no regressions:

image

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

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

added some suggestion but overall

LGTM

ty

@layershifter layershifter merged commit 9b99a9d into microsoft:master Jul 23, 2024
18 checks passed
@layershifter layershifter deleted the fix/decorators branch July 23, 2024 14:12
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Jul 25, 2024
* master: (77 commits)
  chore(react-examples): replace storybook deprecated api with static stories (microsoft#32074)
  [chore]: create base class for accordion item and remove style and layout specific api (microsoft#32102)
  release: applying package updates - web-components
  chore(web-components): add test for complex focus management (microsoft#32009)
  [chore]: create base class for avatar and remove style and layout specific api (microsoft#32083)
  [chore]: create base class for text input and remove style and layout specific api (microsoft#32080)
  release: applying package updates - web-components
  chore:(react-nav-preview) Scaffold AppItem (microsoft#32088)
  docs(`react-teaching-popover`): Adding subcomponents' API to `TeachingPopover` documentation page (microsoft#32084)
  feat(web-components): relax setTheme() argument type to allow custom tokens (microsoft#32087)
  release: applying package updates - react-components
  [Chore]: Create Spinner base class to abstract out style and layout specific api. (microsoft#32067)
  [Chore]: Create Progress Bar base class to abstract out style and layout specific api. (microsoft#32066)
  fix(react-storybook-addon): transform decorator to function in withAriaLive() (microsoft#32011)
  fix(motion): improve Web Animations API detection in tests (microsoft#32029)
  chore(eslint-plugin): removes type dependency on @fluentui/react-utilities internals in ban-instanceof-html-element rule (microsoft#32072)
  release: applying package updates - react v8
  release: applying package updates - web-components
  Chore: Create Divider base class to abstract out style and layout specific api (microsoft#32065)
  fix(TimePicker): Clear text when date value changes to null (microsoft#31626)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: motion example is flickering in docs
3 participants