Skip to content

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented May 19, 2024

Details

Sets the NSBaselineOffsetAttributeName of the attributedText based on the following formula:

CGFloat baseLineOffset = (maximumLineHeight - maximumFontLineHeight) / 2.0;

The text is vertically centered in the lineHeight of the TextInput (which corresponds to the height of the cursor).
The solution was already added in facebook/react-native:

Related Issues

fixes Expensify/App#17767 fixes Expensify/App#14445 fixes Expensify/App#15640
Related facebook/react-native#35741 facebook/react-native#31112 facebook/react-native#28012 facebook/react-native#33986

Manual Tests

Linked PRs

Copy link

github-actions bot commented May 19, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@fabOnReact
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@fabOnReact
Copy link
Contributor Author

Expensify Composer (When moving to another line, the word and emoji are close to each other):

The difference of baseline is 1.15 (barely noticeable), maximumLineHeight 20, maximumFontLineHeight 17.69

Before After
Before After

@fabOnReact
Copy link
Contributor Author

fabOnReact commented May 20, 2024

Expensify Composer (increasing lineHeight to display more clearly differences in the screenshots):

The result is achieved after setting textInputCompose.lineHeight: 40.

  • baseLineOffset is 11.15
  • maximumLineHeight is 40
  • maximumFontLineHeight is 17.69
Before After
Before After
Before After

@fabOnReact
Copy link
Contributor Author

iOS - Chat - Cursor in the second and following lines touches the symbols of the previous line #15640

Fixes issue Expensify/App#15640

Before After

@tomekzaw
Copy link
Collaborator

Thanks @fabOnReact for this amazing PR!

@tomekzaw
Copy link
Collaborator

@fabOnReact We've reviewed the changes and we'd like to merge this PR but it's still marked as draft

@fabOnReact fabOnReact marked this pull request as ready for review May 21, 2024 09:23
@fabOnReact
Copy link
Contributor Author

@tomekzaw Thanks a lot. I moved the PR to ready for review.

@tomekzaw tomekzaw merged commit b6ba8fe into Expensify:main May 21, 2024
@hungvu193

This comment was marked as resolved.

@fabOnReact

This comment was marked as resolved.

@hungvu193
Copy link

@hungvu193 did you test it?

  • Remove that line of code from Expensify
  • You need to build Expensify and run it on Android
  • Does the bug disappear?

Because the bug is Android only, while this PR introduces changes only to iOS. Thanks a lot!

My apologize for this confusion. I think I mentioned wrong PR here :(

@fabOnReact

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment