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-accordion): deprecate navigation prop #31587

Merged

Conversation

mainframev
Copy link
Contributor

@mainframev mainframev commented Jun 5, 2024

As reported in #30799 , deprecated navigation prop in order to align react-accordion behaviour in the next version with w3 APG pattern

Copy link

codesandbox-ci bot commented Jun 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 5, 2024

📊 Bundle size report

✅ No changes found

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 5, 2024

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme mount 80 87 10 Possible regression
FluentProviderWithTheme virtual-rerender 39 38 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 636 640 5000
Button mount 314 310 5000
Field mount 1098 1088 5000
FluentProvider mount 697 717 5000
FluentProviderWithTheme mount 80 87 10 Possible regression
FluentProviderWithTheme virtual-rerender 39 38 10 Possible regression
FluentProviderWithTheme virtual-rerender-with-unmount 79 74 10
MakeStyles mount 876 860 50000
Persona mount 1764 1700 5000
SpinButton mount 1369 1439 5000
SwatchPicker mount 1555 1552 5000

@mainframev mainframev marked this pull request as ready for review June 5, 2024 18:43
@mainframev mainframev requested a review from a team as a code owner June 5, 2024 18:43
@mainframev mainframev requested review from smhigley, ValentinaKozlova and a team June 5, 2024 18:57
@mainframev mainframev force-pushed the fix/accordion-remove-arrow-nav branch from 98db5e2 to 0a28e71 Compare June 6, 2024 07:03
@mainframev mainframev changed the title fix(react-accordion): align keyboard navigation with APG pattern fix(react-accordion): deprecate navigation prop Jun 6, 2024
Copy link

codesandbox-ci bot commented Jun 6, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

codesandbox-ci bot commented Jun 6, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@mainframev mainframev force-pushed the fix/accordion-remove-arrow-nav branch 2 times, most recently from c172ada to 826aefc Compare June 6, 2024 17:17
@mainframev mainframev self-assigned this Jun 6, 2024
Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

One small comment wording suggestion, & it would probably be good to remove the Navigation example from the storybook (the AccordionNavigation.stories.tsx file)

I think @bsunderhus should be the one to sign off on the overall deprecation approach

@smhigley smhigley requested a review from bsunderhus June 6, 2024 22:45
@Hotell
Copy link
Contributor

Hotell commented Jun 7, 2024

please make sure to update this branch with latest master - accordion was migrated to new structure. that's why there is no codeowner assigned.

Copy link
Contributor

@bsunderhus bsunderhus left a comment

Choose a reason for hiding this comment

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

Thanks for the contrib @mainframev, looks good to me. I'd follow up on @smhigley recommendation and remove the navigation story entirely (https://react.fluentui.dev/?path=/docs/components-accordion--default#navigation)

@bsunderhus bsunderhus force-pushed the fix/accordion-remove-arrow-nav branch from 3308d6e to 23b9b72 Compare June 7, 2024 12:03
@mainframev mainframev enabled auto-merge (squash) June 7, 2024 13:28
@mainframev mainframev force-pushed the fix/accordion-remove-arrow-nav branch from 23b9b72 to 883a05a Compare June 7, 2024 13:29
@mainframev mainframev removed the request for review from smhigley June 10, 2024 08:21
@mainframev mainframev disabled auto-merge June 10, 2024 08:21
@mainframev mainframev removed the request for review from ValentinaKozlova June 10, 2024 08:22
mainframev and others added 2 commits June 10, 2024 10:22
Update packages/react-components/react-accordion/src/components/Accordion/useAccordion.ts

Co-authored-by: Bernardo Sunderhus <bernardo.sunderhus@gmail.com>

Update packages/react-components/react-accordion/src/components/Accordion/Accordion.types.ts

Co-authored-by: Sarah Higley <smhigley@users.noreply.github.com>
@mainframev mainframev force-pushed the fix/accordion-remove-arrow-nav branch from 883a05a to 9db4444 Compare June 10, 2024 08:22
@mainframev mainframev requested a review from a team June 10, 2024 09:12
@smhigley
Copy link
Contributor

@mainframev it looks ready to merge! Thanks for making this change 🎉

@mainframev mainframev merged commit 73c29a7 into microsoft:master Jun 10, 2024
19 checks passed
mainframev added a commit to mainframev/fluentui that referenced this pull request Jun 11, 2024
Co-authored-by: Sarah Higley <smhigley@users.noreply.github.com>
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Jun 11, 2024
…-and-drawer-compat

* master: (43 commits)
  chore: remove react-alert from monorepo (microsoft#31642)
  docs(react-skeleton): extend Skeleton story with SkeletonItem examples (microsoft#31608)
  feat(react-motion): add support for params (microsoft#31566)
  applying package updates
  fix: show default title action in dialog body for modal dialogs (microsoft#31648)
  chore:(react-nav-preview)Remove redundant NavDrawerHeaderNav component. (microsoft#31646)
  Update Accordion Size story to allow collapsing (microsoft#31624)
  fix(react-accordion): deprecate navigation prop (microsoft#31587)
  fix: Drawer story accessibility fixes and docs update (microsoft#31570)
  feat:(react-nav-preview) Adds small size variant (microsoft#31589)
  feat: update divider to use element internals (microsoft#31627)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /3rd batch (microsoft#31601)
  chore:(docs) Adding Jest testing document (microsoft#31554)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /2nd batch (microsoft#31600)
  build(deps): bump tar from 6.1.11 to 6.2.1 (microsoft#31633)
  applying package updates
  fix: allow updating of CSS properties when they are already defined (microsoft#31629)
  fix: corrects the border-color for switch when in the checked state on rest (microsoft#31628)
  chore: update Switch to leverage ElementInternals via Checkbox (microsoft#31613)
  chore: update temporarily codeowners for split-in-two migrated packages to maintain proper PR review assignemnt for outdated branches (microsoft#31616)
  ...
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Jun 11, 2024
* master: (43 commits)
  chore: remove react-alert from monorepo (microsoft#31642)
  docs(react-skeleton): extend Skeleton story with SkeletonItem examples (microsoft#31608)
  feat(react-motion): add support for params (microsoft#31566)
  applying package updates
  fix: show default title action in dialog body for modal dialogs (microsoft#31648)
  chore:(react-nav-preview)Remove redundant NavDrawerHeaderNav component. (microsoft#31646)
  Update Accordion Size story to allow collapsing (microsoft#31624)
  fix(react-accordion): deprecate navigation prop (microsoft#31587)
  fix: Drawer story accessibility fixes and docs update (microsoft#31570)
  feat:(react-nav-preview) Adds small size variant (microsoft#31589)
  feat: update divider to use element internals (microsoft#31627)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /3rd batch (microsoft#31601)
  chore:(docs) Adding Jest testing document (microsoft#31554)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /2nd batch (microsoft#31600)
  build(deps): bump tar from 6.1.11 to 6.2.1 (microsoft#31633)
  applying package updates
  fix: allow updating of CSS properties when they are already defined (microsoft#31629)
  fix: corrects the border-color for switch when in the checked state on rest (microsoft#31628)
  chore: update Switch to leverage ElementInternals via Checkbox (microsoft#31613)
  chore: update temporarily codeowners for split-in-two migrated packages to maintain proper PR review assignemnt for outdated branches (microsoft#31616)
  ...
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request Jun 14, 2024
Co-authored-by: Sarah Higley <smhigley@users.noreply.github.com>
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.

6 participants