-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Carousel: Refactor CarouselAnnouncer #32896
base: master
Are you sure you want to change the base?
Carousel: Refactor CarouselAnnouncer #32896
Conversation
📊 Bundle size report✅ No changes found |
...ents/react-carousel-preview/library/src/components/CarouselAnnouncer/useCarouselAnnouncer.ts
Outdated
Show resolved
Hide resolved
...ents/react-carousel-preview/library/src/components/CarouselAnnouncer/useCarouselAnnouncer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not we just use https://react.fluentui.dev/?path=/docs/utilities-aria-live-useannounce--docs?
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 646 | 647 | 5000 | |
Button | mount | 307 | 301 | 5000 | |
Field | mount | 1131 | 1135 | 5000 | |
FluentProvider | mount | 711 | 729 | 5000 | |
FluentProviderWithTheme | mount | 82 | 84 | 10 | |
FluentProviderWithTheme | virtual-rerender | 36 | 37 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 77 | 82 | 10 | |
MakeStyles | mount | 876 | 860 | 50000 | |
Persona | mount | 1773 | 1797 | 5000 | |
SpinButton | mount | 1403 | 1392 | 5000 | |
SwatchPicker | mount | 1627 | 1664 | 5000 |
change/@fluentui-react-aria-1b519f98-87ae-4fe2-be29-f46d1fe6fec4.json
Outdated
Show resolved
Hide resolved
ab21ee6
to
f81cca8
Compare
f81cca8
to
79ac345
Compare
...react-components/react-carousel-preview/stories/src/Carousel/CarouselActionCards.stories.tsx
Outdated
Show resolved
Hide resolved
packages/react-components/react-carousel-preview/library/src/components/Carousel/useCarousel.ts
Outdated
Show resolved
Hide resolved
|
||
const announcementText = announcement(activeIndex, totalNavLength.current, navGroupRef.current); | ||
|
||
if (announcementTextRef.current === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this will cause bugs in Strict Mode, https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-double-rendering-in-development
Previous Behavior
CarouselAnnouncer was driven via state, causing unnecessary re-renders.
New Behavior
CarouselAnnouncer driven by event updates that directly modify DOM.