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: correct content height in TextKit 1 compatibility mode #363

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

QichenZhu
Copy link
Contributor

Details

The input is sometimes not scrollable on iOS.

Related Issues

$ Expensify/App#41567
PROPOSAL: Expensify/App#41567 (comment)

Manual Tests

  1. Open New Expensify and log in
  2. Tap FAB > Assign task
  3. Enter any title and add a long comment
  4. On the confirmation screen open the the description and scroll down
  5. Finish creating a task
  6. Open the created task and tap the description
  7. Scroll down

Expected Result:
User is able to scroll down the task description.

Linked PRs

Copy link

github-actions bot commented Jun 1, 2024

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

@QichenZhu
Copy link
Contributor Author

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

@QichenZhu
Copy link
Contributor Author

recheck

@QichenZhu QichenZhu marked this pull request as ready for review June 1, 2024 11:46
@tomekzaw
Copy link
Collaborator

tomekzaw commented Jun 1, 2024

@QichenZhu Thanks for the PR!

I might be slightly biased by the length of the code changed but I lean more towards the other solution mentioned in the comment (i.e. switching to TextKit 1 mode earlier by accessing layoutManager field in willMoveToWindow:). This one is a bit shorter and thus easier to understand for me. Are there any other benefits in the current approach?

@QichenZhu
Copy link
Contributor Author

Thanks for your feedback!

I prefer the main solution because it's straightforward - the height changes after the switch, so I just recover it.

The alternative seems somewhat magical. Why does the timing of the switch matter?

Please correct me if I'm wrong. @tomekzaw @hungvu193

@hungvu193
Copy link

I prefer the main solution because it's straightforward - the height changes after the switch, so I just recover it.

I also agree with @QichenZhu at this point.

Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

I confirm this PR resolves the original issue (which btw is also reproducible in the example app). Thanks for the PR!

@tomekzaw tomekzaw merged commit cfa443b into Expensify:main Jun 4, 2024
5 checks passed
@QichenZhu QichenZhu deleted the fix-content-height branch July 4, 2024 02:04
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