fix: IOU - one character descriptions are not saved #343
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
There's a problem here - the diacritic inside a Screen.Recording.2024-05-10.at.13.38.15.mov |
|
@BartoszGrajdek I have added a condition to fix it. |
|
@charles-liang Can you please add screenshots of your tests in PR body? |
src/MarkdownTextInput.web.tsx
Outdated
|
|
||
| if (compositionRef.current) { | ||
| const nativeEvent = e.nativeEvent as MarkdownNativeEvent; | ||
| if (compositionRef.current && (nativeEvent.inputType !== 'insertCompositionText' || nativeEvent.data !== '*')) { |
There was a problem hiding this comment.
I doubt it will function properly if the single character entered is * here
There was a problem hiding this comment.
when it's just a single character '*', the previous condition composition.current is false.
Ok, I will add it today. |
|
@sobitneupane Already added the test of the original issue and the test case your comment mentioned |
@charles-liang Expensify/App#40799 issue arrises specifically when single character is the input. Can you please update your Test Steps and ScreenRecording to reflect that. |
|
@sobitneupane I have already updated the test video to include the content what requested. |
|
Sorry, I was on sick leave last week. I'll take a look today 👀 |
|
@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. |
|
@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. |
|
@sobitneupane i merge to latest main branch and retest it. |
There was a problem hiding this comment.
@charles-liang how did you test the PR? I might be missing something in my testing. I can still reproduce the issue.
src/MarkdownTextInput.web.tsx
Outdated
| updateTextColor(divRef.current, changedText); | ||
| const nativeEvent = e.nativeEvent as MarkdownNativeEvent; | ||
| if (compositionRef.current && (nativeEvent.inputType !== 'insertCompositionText' || nativeEvent.data !== '*')) { | ||
| updateTextColor(divRef.current, e.target.innerText); |
There was a problem hiding this comment.
You have removed the change introduced by this PR.
| updateTextColor(divRef.current, e.target.innerText); | |
| updateTextColor(divRef.current, changedText); |
There was a problem hiding this comment.
@sobitneupane i copy the builded into App/node_modules/@expensify/react-native-live-markdown/lib/commonjs/MarkdownTextInput.web.jsMarkdownTextInput.web.js.
and the test step
Manual Tests
On Android mWeb
- Navigate to staging.new.expensify.com
- Click on FAB > Submit expense > Manual
- Input an amount
- Select a user
- Click on "Description"
"The system keyboard must be set to predictive text mode, or what's called autocomplete mode. Just type a few characters."- Click save when the system's predictive text is active.
Co-authored-by: Sobit Neupane <[email protected]>
|
@charles-liang I replaced the react-native-live-markdown in node-modules with the one from the PR (after vidma_recorder_edited_24052024_144317.mp4 |
src/MarkdownTextInput.web.tsx
Outdated
| @@ -337,14 +338,14 @@ const MarkdownTextInput = React.forwardRef<TextInput, MarkdownTextInputProps>( | |||
| } | |||
| const changedText = e.target.innerText; | |||
|
|
|||
| if (compositionRef.current) { | |||
| const nativeEvent = e.nativeEvent as MarkdownNativeEvent; | |||
| if (compositionRef.current && (nativeEvent.inputType !== 'insertCompositionText' || nativeEvent.data !== '*')) { | |||
There was a problem hiding this comment.
Shouldn't we return early if inputType is insertCompositionText?
There was a problem hiding this comment.
I have reviewed my proposal again, and I apologize for modifying the wrong code while handling conflicting situations.
The condition here is that it only early returns if the composite and type are insertCompositionText, and it is not ** to bold.
…code when merging
…code when merging
|
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 |
|
@BartoszGrajdek I will review it in my end first and will let you know . |
|
We think this might be already fixed in E/App so we're waiting for re-tests of the app. |
|
We can close this PR. The issue linked with the PR is no longer reproducible. |
@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
"The system keyboard must be set to predictive text mode, or what's called autocomplete mode. Just type a few characters."
Linked PRs
Tested
2.mov
default.mov