Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(hooks): minor fixes to useAccessibility & useStyles #2268

Merged
merged 6 commits into from
Jan 27, 2020
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jan 24, 2020

This PR performs two changes.

  • useAccessibility() - we found out based on usage (chore(Box|Button|Image|Avatar): converting components with hooks #2238) that there is no reasons to keep the same reference to returned getA11Props function - there is no reasons to use it as a dependency in other hooks (React.useMemo(() => {}, [getA11Props])). This change is potentially breaking.
  • useState() - useState() was replaced with useReducer() to grant better syncs in Concurrent Mode

@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Jan 24, 2020

Perf comparison

Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.54 0.38 1.42:1 2000 1087
🔧 Button.Fluent 1.26 0.17 7.41:1 1000 1262
🔧 Checkbox.Fluent 1.51 0.3 5.03:1 1000 1506
🔧 Dialog.Fluent 0.33 0.17 1.94:1 5000 1650
🔧 Dropdown.Fluent 3.43 0.42 8.17:1 1000 3425
🔧 Icon.Fluent 0.23 0.04 5.75:1 5000 1151
🔧 Image.Fluent 0.1 0.07 1.43:1 5000 485
🔧 Slider.Fluent 1.86 0.3 6.2:1 1000 1856
🦄 Text.Fluent 0.05 0.16 0.31:1 5000 259
🦄 Tooltip.Fluent 0.33 17.72 0.02:1 5000 1630

🔧 Needs work     🎯 On target     🦄 Amazing

Generated by 🚫 dangerJS

(_prevState: State, nextState: State) => forceUpdate(nextState),
[],
)
const [, forceUpdate] = React.useReducer((c: number) => c + 1, 0) as [never, () => void]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [, forceUpdate] = React.useReducer((c: number) => c + 1, 0) as [never, () => void]
const [, syncState] = React.useReducer((c: number) => c + 1, 0) as [never, () => void]

Or just remove syncState and use forceUpdate

Copy link
Member Author

Choose a reason for hiding this comment

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

We have only forceUpdate() now 👍

mergeProps(slotName, slotProps, latestDefinition.current),
[],
)
return <SlotProps extends Record<string, any>>(slotName: string, slotProps: SlotProps) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for simplifying

@layershifter layershifter merged commit ffab117 into master Jan 27, 2020
@layershifter layershifter deleted the fix/hooks branch January 27, 2020 11:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants