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

Implement iOS "Cancel" button #5376

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Nov 29, 2024

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-28.at.17.04.37.mp4

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

@Jon-edge Jon-edge force-pushed the jon/ios-text-input-cancel branch 5 times, most recently from e988822 to 1bcff4b Compare November 29, 2024 20:35
@@ -172,7 +173,7 @@ export const SimpleTextInput = React.forwardRef<SimpleTextInputRef, SimpleTextIn
if (onSubmitEditing != null) onSubmitEditing()
})

const backIconSize = useDerivedValue(() => interpolate(focusAnimation.value, [0, 1], [0, themeRem]))
const backIconSize = useDerivedValue(() => (Platform.OS === 'ios' ? 0 : interpolate(focusAnimation.value, [0, 1], [0, themeRem])))
Copy link
Contributor

Choose a reason for hiding this comment

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

create an isIos var so we don't send Platform over the bridge

marginLeft: theme.rem(0.5),
justifyContent: 'center',
alignItems: 'center',
height: theme.rem(3.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this height so the input doesn't get fat.

useAnimatedStyle(() => {
'worklet'
return {
width: interpolate(focusAnimation.value, [0, 1], [0, width]),
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't animate layout props because of performance issue. We want to use transform instead to make our animations smooth. If you can't figure out how we can animate this component in a performant way, then we should just not animate the cancel button.

</InputContainerView>
{Platform.OS === 'ios' && (
<CancelButton focusAnimation={focusAnimation}>
<TouchContainer hitSlop={theme.rem(0.75)} accessible onPress={handleDonePress} testID={`${testID}.cancelButton`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Touch component within a touch component? That doesn't seem right. Move this to be a child of ContainerView.

flex: 1
})

const OuterInputContainerView = styled(View)({
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete OuterInputContainerView because it's not needed. We have ContainerView already.

Copy link
Contributor

@samholmes samholmes left a comment

Choose a reason for hiding this comment

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

I added a fixup commit with some of my feedback suggetions. Though, there are still broken scenes remaining:

Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 12 42 03
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 12 41 44
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 12 41 16

Copy link
Contributor

@samholmes samholmes left a comment

Choose a reason for hiding this comment

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

Please fix all the regression that I found by checking ever scene where SimpleTextInput is implemented.

Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 37 09
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 37 03
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 35 39
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 35 13
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 34 54

Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 34 51
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 34 30
Simulator Screenshot - iPhone 15 Pro - 2024-11-29 at 13 33 35

@Jon-edge
Copy link
Collaborator Author

Jon-edge commented Nov 30, 2024

Seems we need to wrap another container around the component since originally it had the default flex properties on the outermost container, while the new container needs different flex properties.

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