Expensify / react-native-live-markdown

Drop-in replacement for React Native's TextInput component with Markdown formatting.
https://www.npmjs.com/package/@expensify/react-native-live-markdown
MIT License
668 stars 45 forks source link

Fix: correct content height in TextKit 1 compatibility mode #363

Closed QichenZhu closed 1 month ago

QichenZhu commented 1 month ago

Details

The input is sometimes not scrollable on iOS.

Related Issues

$ https://github.com/Expensify/App/issues/41567 PROPOSAL: https://github.com/Expensify/App/issues/41567#issuecomment-2137792675

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

github-actions[bot] commented 1 month ago

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

QichenZhu commented 1 month ago

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

QichenZhu commented 1 month ago

recheck

tomekzaw commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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.