feat(react-tags): decouple useTagGroupBase_unstable from Tabster + export contexts#36228
feat(react-tags): decouple useTagGroupBase_unstable from Tabster + export contexts#36228mainframev wants to merge 3 commits into
Conversation
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Avatar Converged 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Avatar Converged.badgeMask.normal.chromium.png | 5 | Changed |
vr-tests-react-components/Charts-DonutChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Charts-DonutChart.Dynamic - Dark Mode.default.chromium.png | 7530 | Changed |
vr-tests-react-components/Menu Converged - submenuIndicator slotted content 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Menu Converged - submenuIndicator slotted content.default.submenus open.chromium.png | 605 | Changed |
| vr-tests-react-components/Menu Converged - submenuIndicator slotted content.default - RTL.submenus open.chromium.png | 404 | Changed |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 849 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 65 | Changed |
vr-tests-react-components/ProgressBar converged 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - High Contrast.default.chromium.png | 66 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png | 41 | Changed |
vr-tests-react-components/TagPicker 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - RTL.chromium.png | 635 | Changed |
There were 1 duplicate changes discarded. Check the build logs for more information.
mainframev
force-pushed
the
feat/headless-tag-1-refactor
branch
from
99f8a77 to
24a9f85
Compare
mainframev
force-pushed
the
feat/headless-tag-1-refactor
branch
from
24a9f85 to
4af8734
Compare
mainframev
force-pushed
the
feat/headless-tag-1-refactor
branch
from
4af8734 to
08cb5a5
Compare
| export const useTagGroupBase_unstable = ( | ||
| props: TagGroupBaseProps, | ||
| ref: React.Ref<HTMLDivElement>, | ||
| options?: UseTagGroupBaseOptions, |
There was a problem hiding this comment.
Adding third argument is an architecture change. I would like to avoid it
There was a problem hiding this comment.
good callout , do we have this api change in any of our existing base hooks ? if not this should be avoided
There was a problem hiding this comment.
There was a problem hiding this comment.
true, but it leaked from the similar PR #36087 and should be undone
There was a problem hiding this comment.
we will ship lint rule for this to prevent these regressions
| role, | ||
| 'aria-disabled': disabled, | ||
| ...arrowNavigationProps, | ||
| ...options?.arrowNavigationProps, |
There was a problem hiding this comment.
This could be just spreaded to props in useTagGroup_unstable, no?
| @@ -48,26 +53,7 @@ export const useTagGroupBase_unstable = ( | |||
|
|
|||
| const handleTagDismiss: TagGroupBaseState['handleTagDismiss'] = useEventCallback((e, data) => { | |||
| onDismiss?.(e, data); | |||
There was a problem hiding this comment.
useTagGroup_unstable can just enhance onDismiss callback?
layershifter
left a comment
layershifter
left a comment
There was a problem hiding this comment.
Let's avoid UseTagGroupBaseOptions & options param to keep our hooks consistent
Hotell
left a comment
Hotell
left a comment
There was a problem hiding this comment.
left some comments - needs changes/justification if there is no other way. ty
| export const useTagGroupBase_unstable = ( | ||
| props: TagGroupBaseProps, | ||
| ref: React.Ref<HTMLDivElement>, | ||
| options?: UseTagGroupBaseOptions, |
There was a problem hiding this comment.
good callout , do we have this api change in any of our existing base hooks ? if not this should be avoided
| import type { TagValue } from '../../utils/types'; | ||
| import type { TabsterDOMAttribute } from '@fluentui/react-tabster'; | ||
|
|
||
| type UseTagGroupBaseOptions = { |
There was a problem hiding this comment.
why cant we extend BaseProps type with this and have it all passed through props without need to change hook api pattern (which we probably should not do) ?
4 participants
Stack
This PR is part of a 3-PR stack. Review and merge bottom-up:
master) — merge firstSummary
useTagGroupBase_unstablefrom@fluentui/react-tabsterand@fluentui/react-shared-contexts. Arrow-key navigation and post-dismiss focus restoration are now pluggable via a new optionalUseTagGroupBaseOptionsargument (arrowNavigationProps,onAfterTagDismiss), mirroring theuseMenuTriggerBase_unstablepattern. The styleduseTagGroup_unstablekeeps full behaviour by supplying Tabster-backed implementations through that options bag.TagGroupContextProvider/useTagGroupContext_unstable,InteractionTagContextProvider/useInteractionTagContext_unstable, and theTagGroupContextValue/InteractionTagContextValuetypes. Also re-exports the previously-missingTagContextValuestype.