Skip to content

Port windows fork of Pressable to win32 #9132

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

Merged
merged 14 commits into from
Jan 8, 2022

Conversation

Saadnajmi
Copy link
Contributor

@Saadnajmi Saadnajmi commented Nov 14, 2021

Description

Type of Change

  • New feature (non-breaking change which adds functionality)

Why

Copy the windows port of Pressable to win32. This port adds extra desktop functionality like keyboard focus state and mouse hover state support. This is primarily so that FluentUI React Native can use Pressable downstream, and remove it's own copy of Pressability (microsoft/fluentui-react-native#743).

What

What changes were made to the codebase to solve the bug, add the functionality, etc. that you specified above.

I simply wholesale copied Pressable.windows.js to Pressable.win32.js, along with the PressableExample page in rn-tester. I also added two lines to the test to test that onHoverIn/onHoverOut are indeed registered.

Screenshots

Screenshot 2021-12-17 133056

Testing

Added test pages for Pressable, which is also a copy of the windows one.

Microsoft Reviewers: Open in CodeFlow

@Saadnajmi Saadnajmi requested review from a team as code owners November 14, 2021 17:28
@Saadnajmi Saadnajmi marked this pull request as draft November 14, 2021 18:42
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Consistency between platforms is 👍

@NickGerleman
Copy link
Contributor

Running "yarn lint:fix" should resolve CI errors. If you plan to do long term RNW dev, the eslint extension for VSCode is a must.

@PPatBoyd
Copy link
Contributor

how will we stay consistent with changes? Can we instead make a common file that's exported from the platform-specific files?

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Dec 11, 2021

how will we stay consistent with changes? Can we instead make a common file that's exported from the platform-specific files?

For staying in sync with upstream, it looks like react-native-platform-override should take care of it. Notice the overrides.json changes, and the hashes.

For instead making a common file that's exported, I haven't seen that done by other win32 components here. My guess is it's easier to keep track of overrides that way, and theoretically anytime windows needs to make a change bc of an upstream change, the same tooling should tell us that win32 needs a change.

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Dec 11, 2021

Running "yarn lint:fix" should resolve CI errors. If you plan to do long term RNW dev, the eslint extension for VSCode is a must.

(Somehow missed this message till now)

Thanks! I'll update with both of those things.
I also had issues actually running the test app, so I'll probably reach out for more specific questions on what should be overridden / how to run the test app properly when I come back to this change.

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Dec 12, 2021

EDIT: Looks like I have most of my issues with this PR fixed, but I still am not getting hover events :( . I'm not sure how to attach a debugger in this repo properly or if I need a devmain enlisted machine for things to behave properly. I understand the windows team is out for the holidays, so I'll pick this up and figure out who to bug to answer my questions when people are more available :D

@Saadnajmi
Copy link
Contributor Author

Huzzah! Hover events :D

image

@Saadnajmi Saadnajmi marked this pull request as ready for review December 22, 2021 04:25
@Saadnajmi
Copy link
Contributor Author

New year, new ping! Would someone help me understand why the lint check is failing? The types it's complaining about definitely exist..

@acoates-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@acoates-ms
Copy link
Contributor

Looks like its complaining about some of the Key events not being on view? That type exists on the windows fork of CoreEventTypes, but not any win32 fork of that file.

@Saadnajmi
Copy link
Contributor Author

Looks like its complaining about some of the Key events not being on view? That type exists on the windows fork of CoreEventTypes, but not any win32 fork of that file.

Right, I did fork the CoreEvent types to win32 as well with my last commit (which includes KeyEvent but no dice.

Am I correct to assume that react-native-win32 doesn't also inherit overrides from react-native-windows, so any overrides from Core you want to do would need to be done twice?

@acoates-ms
Copy link
Contributor

Yes, any overrides need to be done for win32 and windows separately. Did you add the Key types to view.win32.js too? Just adding the types to CoreEventTypes wouldn't do anything unless they were added to view.

@Saadnajmi
Copy link
Contributor Author

Saadnajmi commented Jan 7, 2022

Yes, any overrides need to be done for win32 and windows separately. Did you add the Key types to view.win32.js too? Just adding the types to CoreEventTypes wouldn't do anything unless they were added to view.

Ah, that's probably it. Looking at it, I probably want to use ViewWin32.tsx not view.win32.js . The former already includes types for key events, and I'm not sure what the purpose of the latter is. Thanks, I'll try that!

EDIT: Actually, I'll copy the overrides from view.windows.js, since this is all Flow. I'm still not quite sure what the difference between View vs ViewWindows/ViewWin32 is, but I'll assume it's just for typing. https://github.com/microsoft/react-native-windows/blob/main/vnext/src/Libraries/Components/View/View.windows.js

@Saadnajmi
Copy link
Contributor Author

Yes, any overrides need to be done for win32 and windows separately. Did you add the Key types to view.win32.js too? Just adding the types to CoreEventTypes wouldn't do anything unless they were added to view.

Ah, that's probably it. Looking at it, I probably want to use ViewWin32.tsx not view.win32.js . The former already includes types for key events, and I'm not sure what the purpose of the latter is. Thanks, I'll try that!

EDIT: Actually, I'll copy the overrides from view.windows.js, since this is all Flow. I'm still not quite sure what the difference between View vs ViewWindows/ViewWin32 is, but I'll assume it's just for typing. https://github.com/microsoft/react-native-windows/blob/main/vnext/src/Libraries/Components/View/View.windows.js

I added key types to View.win32.js, and also forked ViewPropTypes.win32.js, but I'm still seeing the same errors. I'm not sure why flow doesn't think the types exists. I think I've overridden everything properly..

@NickGerleman
Copy link
Contributor

@Saadnajmi looking at the error, I think the Flow type checker might be using the non .win32 variants of the files. In the .flowconfig file, there is a section where we tell the type-checker to ignore certain shared files, to force it to resolve the forked version. I think you may need to add the shared files you forked to that list.

@Saadnajmi
Copy link
Contributor Author

@Saadnajmi looking at the error, I think the Flow type checker might be using the non .win32 variants of the files. In the .flowconfig file, there is a section where we tell the type-checker to ignore certain shared files, to force it to resolve the forked version. I think you may need to add the shared files you forked to that list.

Looks like that was it! Thank you =)

@Saadnajmi Saadnajmi merged commit 840a5ac into microsoft:main Jan 8, 2022
@Saadnajmi Saadnajmi deleted the pressable-win32 branch January 8, 2022 01:41
Saadnajmi added a commit to Saadnajmi/react-native-windows that referenced this pull request Jan 8, 2022
* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig
Saadnajmi added a commit to Saadnajmi/react-native-windows that referenced this pull request Jan 8, 2022
* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig
Saadnajmi added a commit to Saadnajmi/react-native-windows that referenced this pull request Jan 8, 2022
* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig
Saadnajmi added a commit to Saadnajmi/react-native-windows that referenced this pull request Jan 8, 2022
* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig
@Saadnajmi Saadnajmi restored the pressable-win32 branch January 8, 2022 04:02
@Saadnajmi Saadnajmi deleted the pressable-win32 branch January 8, 2022 04:03
Saadnajmi added a commit that referenced this pull request Jan 10, 2022
* Port windows fork of Pressable to win32 (#9132)

* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig

* Update overrides

* yarn lint:fix
Saadnajmi added a commit that referenced this pull request Jan 10, 2022
* Port windows fork of Pressable to win32 (#9132)

* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig

* update overrides

* fix changefiles

* yarn lift:fix

* update overrides

* fix lint error
Saadnajmi added a commit that referenced this pull request Jan 10, 2022
* Port windows fork of Pressable to win32 (#9132)

* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig

* redo overrides from windows fork

* yarn lint:fix

* update overrides

* fix lint issue
NickGerleman pushed a commit that referenced this pull request Jan 10, 2022
* Port windows fork of Pressable to win32 (#9132)

* Port windows Pressable override to win32

* Change files

* Add override

* lint fix

* More overrides, more windows comments

* Change files

* fix overrides

* Add win32 to HoverState

* Overrride types maybe?

* Add keyboarding types to View.win32.js

* override ViewPropTypes.win32.js

* fix flowconfig

* redo overrides from windows fork

* yarn lint:fix + some lines I forgot to copy

* move to other path

* validate-overrides

* yarn lint:fix again

* forgot some props

* move one more file

* lint:fix once more
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.

4 participants