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

[iOS] add caretHeight and caretYoffset to TextInput component #37147 #58

Open
wants to merge 4 commits into
base: ExpensifyRC1-0.72.0-alpha.0
Choose a base branch
from

Conversation

OlimpiaZurek
Copy link

Upstream PR Link

facebook#37147

Summary

This PR adds support for the caretHeight and caretPosition properties in iOS multi-line TextInput. Based on these values, we can calculate the caret position and height and adjust them in the appl so that the cursor does not overlap the previous line of text.

Changelog

[IOS] [ADDED] - caretHeight and caretPosition for multiline TextInput component

Test Plan

Default caret:

default_caret.mov

Caret with caretHeight and caretYOffsetadjustment:

adjusting_caret.mov

@@ -38,6 +38,8 @@ void RCTCopyBackedTextInput(
toTextInput.keyboardAppearance = fromTextInput.keyboardAppearance;
toTextInput.spellCheckingType = fromTextInput.spellCheckingType;
toTextInput.caretHidden = fromTextInput.caretHidden;
toTextInput.caretYOffset = fromTextInput.caretYOffset;
toTextInput.caretHeight = toTextInput.caretHeight;
Copy link

Choose a reason for hiding this comment

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

shouldn't be toTextInput.caretHeight = fromTextInput.caretHeight here?

Copy link
Author

Choose a reason for hiding this comment

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

good catch! Fixed :-)

Copy link

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

Adding myself as a reviewer!

@Santhosh-Sellavel
Copy link

Looking good so far, I'll test it out tomorrow.

Sorry missing track of these, can anyone mention me here so I can pin the notifications to keep track of it.

@alex-mechler
Copy link

can anyone mention me here so I can pin the notifications to keep track of it.

@Santhosh-Sellavel here's your ping

@Santhosh-Sellavel
Copy link

Looks great!

Simulator.Screen.Recording.-.iPhone.14.-.2023-05-31.at.00.52.53.mp4

Copy link

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

What's wrong here with the checklist?

Screenshot 2023-05-31 at 12 55 55 AM

Otherwise, LGTM tests well @alex-mechler!

@alex-mechler
Copy link

Hmm, I have not seen that before. Looks like its normally skipped. Will ask around

@@ -368,6 +368,19 @@ type IOSProps = $ReadOnly<{|
* @platform ios
*/
smartInsertDelete?: ?boolean,
/**

Choose a reason for hiding this comment

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

NAB: Missing a blank line before this comment

@@ -13,6 +13,41 @@
#import <React/RCTBackedTextInputDelegateAdapter.h>
#import <React/RCTTextAttributes.h>

//the UITextSelectionRect subclass needs to be created because the original version is not writable

Choose a reason for hiding this comment

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

NAB:

Suggested change
//the UITextSelectionRect subclass needs to be created because the original version is not writable
// the UITextSelectionRect subclass needs to be created because the original version is not writable

@alex-mechler
Copy link

Tested well, we can ignore the danger check here.

@OlimpiaZurek Can you update the base to be Expensify-0.71.2-alpha.4 to make this easier to deploy?

@AndrewGable
Copy link

Please target ExpensifyRC1-0.72.0-alpha.0 as this is the version we plan to release. Thanks!

@Santhosh-Sellavel
Copy link

@OlimpiaZurek bump!

1 similar comment
@Santhosh-Sellavel
Copy link

@OlimpiaZurek bump!

@Santhosh-Sellavel
Copy link

@alex-mechler Can you give a bump on any internal channels for callstack!

@alex-mechler
Copy link

Actually, we might not need to, based on. https://expensify.slack.com/archives/C01GTK53T8Q/p1686159512184629. We are moving towards deprecating our fork of RN

@AndrewGable
Copy link

FYI I have bumped the upstream PR with Meta

@Santhosh-Sellavel
Copy link

@AndrewGable Can you bump again?

@AndrewGable
Copy link

@OlimpiaZurek - Can you fix the upstream PR's conflicts and then I can reach out again? Thanks.

@AndrewGable
Copy link

Or @koko57 since you might have access to the fork? Thanks

@koko57
Copy link

koko57 commented Jul 24, 2023

Or @koko57 since you might have access to the fork? Thanks

Yes, I will take care of this tomorrow 😊

@OlimpiaZurek OlimpiaZurek changed the base branch from Expensify-0.71.2-alpha.3 to ExpensifyRC1-0.72.0-alpha.0 July 25, 2023 11:25
@koko57
Copy link

koko57 commented Jul 25, 2023

@AndrewGable all conflicts are resolved in the upstream PR, the target branch is changed here for ExpensifyRC1-0.72.0-alpha.0 and all conflicts here are resolved as well

@AndrewGable
Copy link

Thank you, bumped the upstream PR for a review via Discord!

@Santhosh-Sellavel
Copy link

@AndrewGable Still no review. Can you bump them again, thanks!

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.

6 participants