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

Avoid setting a lineHeight lower of the fontSize lineHeight with Text and TextInput #44614

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented May 20, 2024

Summary:

Avoids setting a lineHeight lower of the fontSize lineHeight with Text and TextInput.
The check was previously introduced on the Paper Renderer with PR #37465.

fixes Expensify/App#14445 fixes #29507
related #38359 and Expensify/react-native-live-markdown#350

Changelog:

[IOS] [FIXED] - Avoid setting a lineHeight lower of the fontSize lineHeight with Text and TextInput

Test Plan:

CLICK TO OPEN SOURCE CODE

/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @format
 * @flow
 */

import type {RNTesterModuleInfo} from './types/RNTesterTypes';

import * as React from 'react';
import {StyleSheet, Text, TextInput, View} from 'react-native';

// RNTester App currently uses in memory storage for storing navigation state

const RNTesterApp = ({
  testList,
}: {
  testList?: {
    components?: Array<RNTesterModuleInfo>,
    apis?: Array<RNTesterModuleInfo>,
  },
}): React.Node => {
  return (
    <View style={styles.container}>
      <Text>This is some Text</Text>
      <Text style={styles.text}>
        <Text>jjjj</Text>
        {'\n'}
        <Text>💀</Text>
      </Text>
      <Text>This is some TextInput</Text>
      <TextInput multiline style={styles.input} />
    </View>
  );
};

export default RNTesterApp;

const styles = StyleSheet.create({
  container: {
    marginTop: 250,
  },
  text: {
    lineHeight: 14,
    borderWidth: 1,
  },
  input: {
    lineHeight: 14,
    borderWidth: 1,
  },
});

Before After

Expensify

Before After

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 20, 2024
@fabOnReact fabOnReact changed the title Add check line height Avoid setting a lineHeight lower than the fontSize lineHeight on Text and TextInput May 20, 2024
@fabOnReact fabOnReact changed the title Avoid setting a lineHeight lower than the fontSize lineHeight on Text and TextInput Avoid setting a lineHeight lower of the fontSize lineHeight with Text and TextInput May 20, 2024
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,542,749 -4
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,912,802 -15
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 93c079b
Branch: main

@fabOnReact
Copy link
Contributor Author

fabOnReact commented May 21, 2024

RN-Tester - lineHeight examples

There is a difference in the RNTester test results.

  • I believe this PR introduces the correct behavior
  • The changes were previously merged and approved with PR #37465
  • PR 37465 was reverted for the slight difference in the text baseline, but no issues were reported with the changes in the lineHeight of the text/textinput.
Before After
Before After

@facebook-github-bot
Copy link
Contributor

@sammy-SC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
3 participants