Refactor PressablePropsExtended #2157
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Saadnajmi
changed the title
10 tasks
Collaborator
Actually I'm fairly certain it's IButtonProps -> ButtonProps, ICheckboxProps -> CheckboxProps, etc... So you might want to make sure the name doesn't clash |
rurikoaraki
approved these changes
Sep 22, 2022
Saadnajmi
changed the title
Saadnajmi
changed the title
Saadnajmi
changed the title
Collaborator
Author
|
@rurikoaraki I think I've gotten through the rest of the type errors. The main changes since your last review:
|
rurikoaraki
reviewed
Sep 26, 2022
| info.updateSelectedTabsItemRef && componentRef && info.updateSelectedTabsItemRef(componentRef); | ||
| } | ||
| }, []); | ||
| }, [componentRef, info, itemKey]); |
Collaborator
There was a problem hiding this comment.
post commit review: Based on the comment I'm pretty sure the dependency array is meant to be empty, since the intent is to run it once post-render.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EDIT: Refactored this PR to a different approach. Will update description shortly.
Platforms Impacted
Description of changes
Related to #1035
One downside of switching a bunch of components' root slots' to Pressable is the type goes from
IViewPropstoPressableProps. The former includes the extra platform specific props. Let's try to fix that withPressablePropsExtended!This change does a few things:
Pulls out
usePressableStateand its types into their own files, so they are no longer sharing types / implementation withuseAsPressable. This should make it easier to eventually deprecate the latter and make sure that components aren't accidentally using a type fromuseAsPressable(I had a lot of type issues from this mismatch)Refactors
PressablePropsExtendedto be kinda like what IViewProps is for ViewProps: ViewProps + the extra props from the desktop forks. I'll admit it's not the most elegant implementation, but I think it'll work for our purposes.Refactor Tabs / Experimental Tabs to use the new types. This is where I spent a lot of work to make sure none of the old I-prefixed types were still being used. I also went ahead and fixed some eslint hook warnings while I was at it.
Verification
CI should pass. Also can try locally applying change to others like #2147 to make sure the types work.
Pull request checklist
This PR has considered (when applicable):