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

Switch Tabs components to use React Native Pressable #2145

Merged
merged 7 commits into from
Sep 21, 2022

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Sep 21, 2022

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Following up from #1070, let's move over components off of useAsPressable / our fork of Pressable and onto Pressable from React Native, which contains the newer support for pointer events / android ripple / codegen commands.

Verification

Hover, Focus, and Press work as expected on macOS. E2E testing on CI should confirm as much for windows/win32, but I'll try and test it out myself too.

Screen.Recording.2022-09-21.at.2.33.39.AM.mov

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@@ -146,7 +146,7 @@ export interface TabsItemInfo {
}

export interface TabsItemSlotProps {
root: React.PropsWithRef<IViewProps>;
root: React.PropsWithRef<PressableProps>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically we lose the adapter props here. Will have to think about that. Could probably easily make an "IPressableProps" if I really wanted to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah in practice as long as the props we expose for the component stay constant I think we're ok... Even if props that don't match the type are passed in, they're still on the object so they should get passed through, they just will make TS angry if someone tries to modify the root slot directly and add props that don't exist on PressableProps

Copy link
Collaborator

Choose a reason for hiding this comment

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

So really it'll be more of a problem for us than for clients.

Copy link
Collaborator Author

@Saadnajmi Saadnajmi Sep 21, 2022

Choose a reason for hiding this comment

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

I think type safety is still a but of an open problem in FURN. Adapters is a step towards it but if we enable platforms specific type safety checking via rnx-kit, there's still issues. I'll try to spend a day thinking about how best to not regress the experience for ourselves.

@Saadnajmi Saadnajmi changed the title [Draft] Switch Tabs components to Pressable from React Native Switch Tabs components to Pressable from React Native Sep 21, 2022
@Saadnajmi Saadnajmi marked this pull request as ready for review September 21, 2022 09:38
@Saadnajmi Saadnajmi requested a review from a team as a code owner September 21, 2022 09:38
@Saadnajmi Saadnajmi changed the title Switch Tabs components to Pressable from React Native Switch Tabs components to use React Native Pressable Sep 21, 2022
@rurikoaraki
Copy link
Collaborator

I can test it for win32/windows for you.

@Saadnajmi
Copy link
Collaborator Author

I can test it for win32/windows for you.

Today is "set up computers day" so I should be able to do it, but if it's easy enough after I change the slot, I appreciate it!

@Saadnajmi Saadnajmi merged commit 7f4c8e9 into microsoft:main Sep 21, 2022
@Saadnajmi Saadnajmi deleted the pressable-tab branch September 21, 2022 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants