[Draft] Switch Button, Checkbox, and RadioButton from View to Pressable #1070
Conversation
Saadnajmi
requested review from
rurikoaraki,
harrieshin,
JasonVMo and
HeyImChris
| @@ -96,9 +96,9 @@ const pressable: React.FunctionComponent = () => { | |||
| const [hoverProps, hoverState] = useHoverState({}); | |||
|
|
|||
| return ( | |||
| <Stack horizontal gap={5}> | |||
There was a problem hiding this comment.
This made the Pressable test page look weird, where one square was off center. Removing both props fixed the issue.
| <Pressable renderStyle={renderStyle}> | ||
| <Pressable style={renderStyle}> |
There was a problem hiding this comment.
This prop was named as such because of an issue where the types for react native didn't support a function that returns a style. That's no longer the case, so we can just make this match ViewProps/PressableProps
| onFocus={[Function]} | ||
| onPress={[Function]} | ||
| onMouseEnter={[Function]} | ||
| onMouseLeave={[Function]} | ||
| onResponderGrant={[Function]} |
There was a problem hiding this comment.
not sure why onPress is disappearing from the snapshot. The radio buttons still seem to update for me onPress. This probably requires investigation.
There was a problem hiding this comment.
Actually.. I think it's because onPress got absorbed by Pressability, and doesn't show up as a prop on the final rendered component anymore. This also explains why onMouseEnter & onMouseLeave show up: Pressability spits those out as event handlers
There was a problem hiding this comment.
Nevermind, testing on win32, none of the onPress events are working. I'm going to mark this as a draft, and split out a bug fix to a separate review.
|
An update: This PR requires upstream changes in react-native-windows/react-native-win32, and possibly react-native-macos to properly support the hover and focus states that the FURN Pressable currently does. Since FHL week is over, I will leave this PR as a draft for when it can be picked up again: I still think it's a virtuous addition that would improve the state of the desktop ports (by extending pressable) and lower the bundle size of FURN by removing our port of Pressability |
Saadnajmi
changed the title
|
Closing as this will be split into different PRs |
Platforms Impacted
TODO
onClickprop of experimental button toonPress, to match Pressable.Description of changes
This will handle part of #1035
Before we were using React Native 0.63 (where Pressable was added), we ported over Pressable to our own repo, and created the hook
useAsPressableto get similar functionality. Now that we are on RN 0.63, we should just use the Pressable coming from React Native Core (which has benefits like automatically adding the ripple for android), and remove our port. PR #863 introduced the hookusePressableStateto help with this.The original plan was to switch all components that called
useAsPressableto instead callusePressableState, switch all the root slots fromViewtoPressable, and then delete our port of Pressable.Unfortunately, after investigation, I found out that the RN Core Pressable does not support hover event handlers (I filed facebook/react-native#32406 to track this), nor does it support keyboard focus events on win32. As such, we will continue to have to use our own port of Pressable for at least win32.
As such the new plan is to have our package
@fluentui-react-native/pressableexport the RN Core Pressable for all platforms except win32, which will continue to use our port ofuseAsPressable. This way, we can get the benefits of RN Core press ability (like the ripple on Android), while also keeping our hover/focus/press functionality on all platforms. Once the proper upstream changes have been made in react-native and/or it's desktop ports to Pressable, we can revisit this.This PR switches over Button, Checkbox, and RadioButton. I had issues with porting ContextualMenu, Link, and Tabs/tabItem, so I will leave those for a separate PR.
Verification
I tested on macOS that we still get the proper hover, focus, and press effects. I need to test on other platforms, namely win32 and android to make sure nothing else broke there. Ideally, there should be functionality change.
Pull request checklist
This PR has considered (when applicable):