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
787 stars 56 forks source link

fix: IOU - one character descriptions are not saved #343

Closed charles-liang closed 3 months ago

charles-liang commented 4 months ago

@sobitneupane @BartoszGrajdek

Details

This PR avoid the incorrect behavior of the keyboard during auto-complete on Android mWeb. The main changes are: Add a condition, when the startComposition event trigger, whether the type of text change is 'insertCompositionText'. If confirmed, then execute the logic of Composition.

Related Issues

Issues IOU - one character descriptions are not saved Proposal

Manual Tests

On Android mWeb

  1. Navigate to staging.new.expensify.com
  2. Click on FAB > Submit expense > Manual
  3. Input an amount
  4. Select a user
  5. Click on "Description" "The system keyboard must be set to predictive text mode, or what's called autocomplete mode. Just type a few characters."
  6. Click save when the system's predictive text is active.

Linked PRs

Tested

  1. Mac Chrome/Safari

https://github.com/Expensify/react-native-live-markdown/assets/13301734/c13b79ec-e01e-4088-935f-038291ffd83e

  1. Android mWeb

https://github.com/Expensify/react-native-live-markdown/assets/13301734/dc1d3f1d-b718-4945-b2df-40538392f200

github-actions[bot] commented 4 months ago

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

charles-liang commented 4 months ago

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

BartoszGrajdek commented 4 months ago

There's a problem here - the diacritic inside a bold block doesn't work (video below)

https://github.com/Expensify/react-native-live-markdown/assets/41169160/d44f49b3-78b9-4d1a-bded-720ff2881c1d

charles-liang commented 4 months ago

@BartoszGrajdek I have added a condition to fix it. Are there any test cases you can share with me for testing together? In order to reducing the cost of communication?

sobitneupane commented 4 months ago

@charles-liang Can you please add screenshots of your tests in PR body?

charles-liang commented 4 months ago

@charles-liang Can you please add screenshots of your tests in PR body?

Ok, I will add it today.

charles-liang commented 4 months ago

@sobitneupane Already added the test of the original issue and the test case your comment mentioned

sobitneupane commented 4 months ago
  1. Navigate to staging.new.expensify.com
  2. Click on FAB > Submit expense > Manual
  3. Input an amount
  4. Select a user
  5. Click on "Description" "The system keyboard must be set to predictive text mode, or what's called autocomplete mode. Just type a few characters."
  6. Click save when the system's predictive text is active.

@charles-liang https://github.com/Expensify/App/issues/40799 issue arrises specifically when single character is the input. Can you please update your Test Steps and ScreenRecording to reflect that.

charles-liang commented 4 months ago

@sobitneupane I have already updated the test video to include the content what requested.

BartoszGrajdek commented 4 months ago

Sorry, I was on sick leave last week. I'll take a look today 👀

sobitneupane commented 4 months ago

@charles-liang It seems there might have been an issue while resolving the conflict. Could you please take another look at the code and address the problem? Thanks!

You probably had merge conflict with this PR.

sobitneupane commented 4 months ago

@charles-liang I tried to resolve the conflict and test it in my end. But it doesn't solve the issue. In fact it introduces a new problem: when I go to any chat in mWeb/chrome and start typing, no letters are displayed on the Composer.

charles-liang commented 4 months ago

@sobitneupane i merge to latest main branch and retest it.

sobitneupane commented 4 months ago

@charles-liang I replaced the react-native-live-markdown in node-modules with the one from the PR (after yarn prepare), but I can still reproduce the issue.

https://github.com/Expensify/react-native-live-markdown/assets/25876548/0ec49610-c493-4550-8a52-d3dd30eefdce

BartoszGrajdek commented 4 months ago

Hi! Not sure if you want me to review the PR yet since it's still being worked on. Let me know what's the status of it 👀 @sobitneupane

sobitneupane commented 4 months ago

@BartoszGrajdek I will review it in my end first and will let you know .

tomekzaw commented 3 months ago

We think this might be already fixed in E/App so we're waiting for re-tests of the app.

sobitneupane commented 3 months ago

We can close this PR. The issue linked with the PR is no longer reproducible.