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

Remove deprecated components #3230

Open
wants to merge 13 commits into
base: next
Choose a base branch
from

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Nov 20, 2024

Description

removes the following components:

  • DrawerLayout
  • Swipeable
  • BetterHorizontalDrawer draft component
  • Swipeable draft component

These components were also removed from all files referencing them,
such as examples, index files, and the App.tsx of the common example app

Test plan

  • see how these components are no longer accessible
  • see how there are no errors, and no new warnings thrown as compared to the main branch

@latekvo latekvo changed the base branch from main to next November 25, 2024 10:37
@latekvo latekvo marked this pull request as ready for review November 25, 2024 14:06
@j-piasecki
Copy link
Member

There are still references to the removed components (like in babel.config.js), there's also still documentation for Swipeable component. Could you recheck to make sure they are all removed?

@latekvo
Copy link
Contributor Author

latekvo commented Nov 28, 2024

there's also still documentation for Swipeable component

My bad, I forgot these changes are merged into the next branch, not main, so they don't trigger the docs publication workflow.

fixed in ad0ff9c

there are still references to the removed components (like in babel.config.js)

Not sure which references you're talking about, in the babel.config.js file there are only references to the ReanimatedSwipeable and ReanimatedDrawerLayout components.

fixed in b9cdf18

@latekvo latekvo force-pushed the @latekvo/remove-deprecated-components branch from 9aa72ba to b9cdf18 Compare December 5, 2024 14:31
Comment on lines 192 to 211
<Text style={styles.buttonText}>
Second info icon will block scrolling
</Text>
{/* Info icon will block interaction with other gesture handlers including
the scrollview handler its a descendant of. This is typical for buttons
embedded in a scrollable content on iOS. */}
<InfoButton disallowInterruption name="second" />
</RectButton>
<View style={styles.buttonDelimiter} />
<RectButton
style={styles.rectButton}
onPress={() => Alert.alert('Third row clicked')}>
<Text style={styles.buttonText}>
This one will cancel when you drag outside
</Text>
{/* Info icon will cancel when you drag your finger outside of its bounds and
then back unlike all the previous icons that would activate when you re-enter
their activation area. This is a typical bahaviour for android but less frequent
for most of the iOS native apps. */}
<InfoButton shouldCancelWhenOutside name="third" />
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to reimplement this using the new components? I'd like to keep something along these lines to make sure this behavior doesn't change accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, will do that

Copy link
Contributor Author

@latekvo latekvo Dec 10, 2024

Choose a reason for hiding this comment

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

done in 56344b0 & 0a87e5e

</Animated.Text>
);
}

export default function Example() {
Copy link
Member

Choose a reason for hiding this comment

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

If this is the yellowish looking one then I guess we can remove it entirely, no? It was supposed to compare both implementations visually and with one implementation gone it kinda lost its purpose.

Copy link
Contributor Author

@latekvo latekvo Dec 6, 2024

Choose a reason for hiding this comment

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

Right now it serves more as a test for the programatic swipeable controls, which encountered regressions several times since release.

Screenshot 2024-12-06 at 14 35 22

I will move these tests to the other swipeable example, there's room for both of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 5599019

Copy link
Member

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

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

Make sure CI passes before merging.

@latekvo latekvo force-pushed the @latekvo/remove-deprecated-components branch 3 times, most recently from 91a8d5d to 0a87e5e Compare December 12, 2024 13:00
@latekvo
Copy link
Contributor Author

latekvo commented Dec 16, 2024

To fix the touchables tslint error, I had to use a ts-ignore
due to a regression in TouchableNativeFeedback added in: #3260
I'll fix that issue separately, but for now i think it's out of the scope of this PR.
Said regression fixed here: #3294

If that isn't an issue, i'll be merging this PR.

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