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.

Sign up for GitHub

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

Jump to bottom

Add support for numberOfLines and maximumNumberOfLines props on iOS and Android #37

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Add support for numberOfLines and maximumNumberOfLines props on iOS and Android #37

merged 1 commit into from
Mar 17, 2023

Conversation

Szymon20000
Copy link

@Szymon20000 Szymon20000 commented Dec 8, 2022 •
edited by roryabraham
Loading

Upstream PR

facebook#35703

Summary

We want to be able to set height of TextArea/TextInput/EditText components on all platforms in the same way. Was is currently missing on iOS is expressing the height of a component in lines and this pr is supposed to fix it. Additionally in some cases we want to set maximum number of lines a "TextArea" like component can have. Because of Different screen sizes and accessibility settings it's sometimes hard to compute how many lines a current content takes so the purpose of maximumNumberOfLines prop is to simplify that process.

iOS How it was implemented?

On Android the native "TextArea" like component (EditText) exposes setLines method that sets height of the component in lines. However, I wasn't able to find anything similar on iOS. What is a common method for both platforms is setting maximum height in lines. On iOS we can set .maximumNumberOfLines property on TextContainer and similarly on Android we can call setMaxLines() method to limit number of lines. So Adding maximumNumberOfLines is not a problem.

I realised that when we add enough number of newLines in order to implement numberOfLines prop using maximumNumberOfLines functionality and did exactly that. Probably it's possible to get a font size and based on that set exact height but I didn't want to miss any edge cases and adding few newlines doesn't seem to be an overhead at all.
Also probably otherwise we would need to take into account things like space between lines, padding...

Android

I exposed setMaxLines in the same way setLines was exposed before so I'm pretty confident it's correct however I still need to test it.

Fabric

I implemented numberOfLines and maximumNumberOfLines similarly to what I did for paper architecture but it turned out that there is one edge case. Whenever you tapped the enter button it added a new line to the text input. Even though NSTextContainer clearly had set maximumNumberOfLines property. I checked what are differences between paper and Fabric implementation when it comes to using NSTextContainer but I wasn't able to spot any difference. Fonts, spaces, other text attributes were exactly the same for every character. In RN docs I found that TextInput already have support for numberOfLines to I checked if the problem occurs also there. Turned out that problem is there too and moreover it's also in paper architecture. I checked differences on how NSContainer is used and spotted a different order of methods called on NSTextContainer, NSTextStorage and NSLayoutManager. After changing the order the problem is gone everywhere.

Changelog

[CATEGORY] [TYPE] - Message

Test Plan

I basically run the RNTester app with and without fabric.

  • test iOS (Paper)
  • test iOS (Fabric)
  • test Android (Paper)
  • test Android (Fabric)

@github-actions
Copy link

github-actions bot commented Dec 8, 2022 •
edited
Loading

Fails
🚫

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against bd04a64

@Szymon20000 Szymon20000 mentioned this pull request Dec 8, 2022
Closed
@Szymon20000 Szymon20000 marked this pull request as ready for review December 19, 2022 12:24
roryabraham
roryabraham requested changes Dec 21, 2022
View reviewed changes
Copy link

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

@Szymon20000 Sorry, but this PR needs to target a different branch, probably best to target https://github.com/Expensify/react-native/tree/Expensify-0.71.0-alpha1 for now. Thanks

@Szymon20000
Copy link
Author

@Szymon20000 Sorry, but this PR needs to target a different branch, probably best to target https://github.com/Expensify/react-native/tree/Expensify-0.71.0-alpha1 for now. Thanks

Not a problem. Will change that tomorrow morning.
Can I already open the same pr in rn repo? Would like to know what rn core team thinks about those changes.

@roryabraham
Copy link

Can I already open the same pr in rn repo?

Totally, no reason to wait

@Szymon20000 Szymon20000 changed the base branch from main to Expensify-0.71.0-alpha1 December 22, 2022 09:31
@Szymon20000
Copy link
Author

Done

@Szymon20000
Copy link
Author

This pr needs some polishing like clang-format but would like to first get some feedback on it.

@mountiny mountiny self-requested a review February 9, 2023 10:32
roryabraham
roryabraham requested changes Feb 12, 2023
View reviewed changes
Copy link

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay in reviewing this @Szymon20000. Can you please update it to target the Expensify-0.71.2-alpha.0 branch?

@Szymon20000
Copy link
Author

Apologies for the long delay in reviewing this @Szymon20000. Can you please update it to target the Expensify-0.71.2-alpha.0 branch?

Sure, I will do it tomorrow morning together with renaming the prop as on 0.71 it's changed to "rows".

@Szymon20000 Szymon20000 changed the base branch from Expensify-0.71.0-alpha1 to Expensify-0.71.2-alpha.1 February 14, 2023 08:39
@Szymon20000
Copy link
Author

Tested it after rebase and rename and it seems to works fine.

@roryabraham
Copy link

roryabraham commented Feb 16, 2023 •
edited
Loading

@Szymon20000 Can you address the CI failures in the upstream PR plz?

@Szymon20000
Copy link
Author

@roryabraham I tested it on Android and it looks like numberOfLines/rows doesn't work there properly.
I checked and it's not caused by my changes. On new arch (Fabirc) numberOfLines/rows only set maximum number of lines the TextInput can have. However on old arch (paper) it sets fixed height in lines. However it looks like it does't set maximum limit so we can exceed number of lines we wanted to set. Do you want me to fix the issue also on Android?
I tested it on RNTester on the main branch in upstream repo.

@Szymon20000
Copy link
Author

Will do my best to fix CI failing tests later today.

@mountiny
Copy link

Do you want me to fix the issue also on Android? I tested it on RNTester on the main branch in upstream repo.

I imagine we would love to get this fixed on Android as well cc @roryabraham

@Szymon20000
android part
4bd87ab 
ios changes

update types, configs

fix NativeTextComponent Props

fix fabric issue with newlines

Apply suggestions from code review

Co-authored-by: Janic Duplessis <janicduplessis@gmail.com>

add example to rntester
@roryabraham roryabraham changed the base branch from Expensify-0.71.2-alpha.1 to Expensify-0.71.2-alpha.2 March 17, 2023 13:55
@roryabraham roryabraham merged commit a96968f into Expensify:Expensify-0.71.2-alpha.2 Mar 17, 2023
@bernhardoj bernhardoj mentioned this pull request Apr 21, 2023
Closed
6 tasks
@bernhardoj bernhardoj mentioned this pull request May 2, 2023
Closed
6 tasks
@mwmsh mwmsh mentioned this pull request May 5, 2023
Closed
6 tasks
@aimane-chnaif
Copy link

Coming from Expensify/App#17368:
This PR caused another bug in android - SVG icon inside Text isn't visible.
Hope new upstream PR will fix all known/unknown bugs including this one 🙏

@Szymon20000
Copy link
Author

I believe the issue was already fixed by me and @roryabraham cherry-picked it. Probably the RN fork with the fix is not yet used in the app.

@aimane-chnaif
Copy link

@Szymon20000 that's great! Can you please link that PR in our forked repo?

@Szymon20000
Copy link
Author

I think that's the one dd492a1

@roryabraham
Copy link

Just for clarity, we've since decided to drop our fork, use the upstream repo instead, and focus on getting PRs merged upstream.

@roryabraham
Copy link

So we should be waiting on facebook#38021 and facebook#35703

@fabioh8010 fabioh8010 mentioned this pull request Jul 5, 2023
Merged
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roryabraham roryabraham roryabraham approved these changes

@mountiny mountiny Awaiting requested review from mountiny

Assignees

@Szymon20000 Szymon20000

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Szymon20000 @roryabraham @mountiny @aimane-chnaif