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

fix(CLNP-1538): adjust lineHeight of iOS TextInput #148

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Conversation

bang9
Copy link
Collaborator

@bang9 bang9 commented Nov 17, 2023

External Contributions

This project is not yet set up to accept pull requests from external contributors.

If you have a pull request that you believe should be accepted, please contact
the Developer Relations team [email protected] with details
and we'll evaluate if we can setup a CLA to allow for the contribution.

For Internal Contributors

CLNP-1538

Description Of Changes

  • Adjusted the vertical alignment of TextInput on iOS

AS-IS

iPhone14

image

iPhone11

image

iPhoneSE

image

TO-BE

iPhone14

image

iPhone11

image

iPhoneSE

image

Types Of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply_

  • Bugfix
  • New feature
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance (ex) Prettier)
  • Build configuration
  • Improvement (refactor code)
  • Test

@bang9 bang9 self-assigned this Nov 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (4f12feb) 13.00% compared to head (c9c253e) 12.99%.

Files Patch % Lines
...react-native/src/components/ChannelInput/index.tsx 0.00% 5 Missing ⚠️
...tive-foundation/src/components/TextInput/index.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   13.00%   12.99%   -0.02%     
==========================================
  Files         328      328              
  Lines        7252     7258       +6     
  Branches     2005     2007       +2     
==========================================
  Hits          943      943              
- Misses       6239     6245       +6     
  Partials       70       70              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bang9 bang9 force-pushed the fix/ios-input-valign branch from e53e22d to 0f95666 Compare November 17, 2023 04:22
Comment on lines -41 to -42
paddingVertical: 8,
paddingHorizontal: 16,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

paddingVertical / paddingHorizontal is not working due to bug in React-Native
facebook/react-native#33564

@@ -15,6 +15,10 @@ const TextInput = React.forwardRef<RNTextInput, Props>(function TextInput(
const variantStyle = colors['ui']['input'][variant];
const inputStyle = editable ? variantStyle.active : variantStyle.disabled;
const underlineStyle = variant === 'underline' && { borderBottomWidth: 2, borderBottomColor: inputStyle.highlight };
const fontStyle = {
...typography.body3,
lineHeight: typography.body3.fontSize ? typography.body3.fontSize * 1.2 : undefined,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While vertical alignment is supported on Android, it is not supported on iOS.
For this reason, if the lineHeight is not adjusted proportionally based on fontSize in TextInput, there may be an issue with misalignment

minHeight: 36,
// Android - padding area is hidden
// iOS - padding area is visible
maxHeight: Platform.select({ ios: 36 * 2 + 16, android: 36 * 2 }),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When padding is applied to the top/bottom of TextInput, on Android, the text may be obscured by the padding area, whereas on iOS, it remains visible.
This results in a discrepancy in the number of visible text lines on the UI (e.g., three lines displayed on Android versus only two on iOS).
To address this, add an additional offset equal to the padding vertical.

@bang9 bang9 force-pushed the fix/ios-input-valign branch from 0f95666 to a7da54d Compare November 17, 2023 04:30
const fontStyle = useMemo(() => {
if (!typography.body3.fontSize) return typography.body3;
// NOTE: iOS does not support textAlignVertical, so we should adjust lineHeight to center the text in multiline TextInput.
return { ...typography.body3, lineHeight: typography.body3.fontSize * 1.275, textAlignVertical: 'center' };
Copy link
Collaborator Author

@bang9 bang9 Nov 17, 2023

Choose a reason for hiding this comment

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

Generally, to obtain accurate height on iOS, a specific formula is required. However, since it's not executable in JavaScript, the magic number 1.2 is used(0.075 is offset for custom input height).

This number aligns reasonably well with the default system font.
However, if the customer employs a different font, the lineHeight must be adjusted.
For this case, a style prop has been added to ChannelInputProps.

@bang9 bang9 force-pushed the fix/ios-input-valign branch from a7da54d to c9c253e Compare November 17, 2023 05:20
@bang9 bang9 merged commit 62f958d into main Nov 17, 2023
6 checks passed
@bang9 bang9 deleted the fix/ios-input-valign branch November 17, 2023 05:28
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.

3 participants