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(ios): line break mode property added for iOS #45968

Conversation

shubhamguptadream11
Copy link
Collaborator

@shubhamguptadream11 shubhamguptadream11 commented Aug 10, 2024

Summary:

Solves this issue: #44107

Changelog:

[IOS] [ADDED] - Line break mode for TextInput components

Test Plan:

  • Test it does not affect existing text components when set default(none) strategy.
  • Tested all the modes: 'wordWrapping' | 'char' | 'clip' | 'head' | 'middle' | 'tail'.
  • Tested it wouldn't affect TextInput if not provided lineBreakMode.
  • Written test case with these props.

@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 Aug 10, 2024
@react-native-bot
Copy link
Collaborator

Fails
🚫

📋 Verify Changelog Format - See Changelog format

Generated by 🚫 dangerJS against 4609727

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 21,354,842 -14
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 24,549,976 -15
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: fcd526d
Branch: main

@shubhamguptadream11 shubhamguptadream11 marked this pull request as ready for review August 10, 2024 14:23
@shubhamguptadream11 shubhamguptadream11 changed the title Fix/line break mode ios fix(ios): line break mode property added for iOS Aug 10, 2024
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 10, 2024
@shubhamguptadream11
Copy link
Collaborator Author

@cortinico @cipolleschi
I understand the current focus is on New Architecture specific bugs. This fixes the issue coming in old and new arch both. Whenever you have some time, I would really appreciate it if you could review this PR. No rush at all—please review it whenever it's convenient for you. Thanks again!

@ecreeth
Copy link
Contributor

ecreeth commented Aug 14, 2024

Why not lineBreakMode?

@shubhamguptadream11
Copy link
Collaborator Author

@ecreeth This is iOS specific props. I followed convention from existing props: lineBreakStrategyIOS

@ecreeth
Copy link
Contributor

ecreeth commented Aug 14, 2024

@ecreeth This is iOS specific props. I followed convention from existing props: lineBreakStrategyIOS

I know, the think is that there's other props without this IOS suffix that are platform specific like: dynamicTypeRamp , minimumFontScale etc.

@shubhamguptadream11
Copy link
Collaborator Author

I thought this makes much more sense by specifying platform in props. But I am okay to rename it.

@shubhamguptadream11
Copy link
Collaborator Author

@cipolleschi @NickGerleman Can you guys please check this PR once?

@cortinico
Copy link
Contributor

@cipolleschi @NickGerleman Can you guys please check this PR once?

As I already mentioned, we're hyperfocused on New Architecture-only bug at the moment so we can't review PRs within a couple of days especially if they're not New Architecture bugfixes.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Hi @shubhamguptadream11, thanks for working on this.
This PR is too big, it will be quite hard to review and land it.

Can I suggest to split it in multiple parts?
Ideally, we can have:

  1. a part that introduce the C++ changes and new functions we need
  2. a part that updates the iOS part to consume those new functions, even though they are not usable from JS
  3. the last part which implement the Js APIs to use the new mode.

@shubhamguptadream11
Copy link
Collaborator Author

shubhamguptadream11 commented Aug 21, 2024

Hi @cipolleschi ,

Thanks for the feedback! I completely understand the concern. I'll split the PR into the suggested parts:

  • C++ changes and new functions
  • iOS updates to consume those new functions (without JS integration)
  • Implementation of the JS APIs for the new mode
    I'll work on breaking it down and submit the smaller, more manageable PR's shortly.

Thanks again for the guidance!

@shubhamguptadream11
Copy link
Collaborator Author

Hi @cipolleschi

I've gone ahead and split the original PR into three separate ones as suggested:

[PR 1] - Introduces the C++ changes and new functions.
[PR 2] - Updates the iOS part to consume the new functions (not yet exposed to JS).
[PR 3] - Implements the JS APIs for the new mode.
Each PR is focused on a specific part of the overall change, making them easier to review and land. Please feel free to review them at your convenience.

facebook-github-bot pushed a commit that referenced this pull request Aug 23, 2024
Summary:
Solves this issue: #44107

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[IOS] [ADDED] - Line break mode for TextInput components. **This includes cpp changes and new functions.**

This PR is a breakdown of [this](#45968) PR.

Pull Request resolved: #46130

Test Plan: - Tested builds in new and old architecture mode.

Reviewed By: andrewdacenko

Differential Revision: D61656894

Pulled By: cipolleschi

fbshipit-source-id: 9a25387cb27cded072e76575e6d2fca01963c621
facebook-github-bot pushed a commit that referenced this pull request Aug 23, 2024
…ons (#46129)

Summary:
Solves this issue: #44107

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[IOS] [ADDED] - Line break mode for TextInput components. **This includes  iOS updates to consume new cpp functions.**

This PR is a breakdown of [this](#45968) PR.

Pull Request resolved: #46129

Test Plan: - Tested builds in new and old architecture mode.

Reviewed By: andrewdacenko

Differential Revision: D61656969

Pulled By: cipolleschi

fbshipit-source-id: 4c6ed983ad15841ce52443bba13962d45c04e756
facebook-github-bot pushed a commit that referenced this pull request Sep 10, 2024
Summary:
Solves this issue: #44107

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[IOS] [ADDED] - Line break mode for TextInput components. **This includes JS APIs for the new mode.**

This PR is a breakdown of [this](#45968) PR.

Pull Request resolved: #46128

Test Plan:
- Added unit tests to cover the new JS APIs.
- Verified that the new mode functions as expected through manual testing.

Reviewed By: andrewdacenko

Differential Revision: D61657004

Pulled By: cipolleschi

fbshipit-source-id: 9fc5c40fc077bee8e1abc51b6eae2e0f0fcd9b8f
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
Development

Successfully merging this pull request may close these issues.

7 participants