-
Notifications
You must be signed in to change notification settings - Fork 65
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(web): handle text input metrics correctly on history undo events #434
base: main
Are you sure you want to change the base?
Conversation
@Skalakid may I ask you (or someone from your team) to review? thank you so much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, left 2 small comments
@hannojg some check are failing, please fix TS and ESlint errors |
Do you know if there is any bump PR in expensify already? |
@hannojg Somehow not all of your commits are signed, can you rebase and sign them, please? |
Signed-off-by: Hanno J. Gödecke <die.drei99@yahoo.de>
Signed-off-by: Hanno J. Gödecke <die.drei99@yahoo.de>
Signed-off-by: Hanno J. Gödecke <die.drei99@yahoo.de>
Signed-off-by: Hanno J. Gödecke <die.drei99@yahoo.de>
29ca4c7
to
894790d
Compare
…n into fix/web-onchange-history-evenst
The merge-base changed after approval.
Lets wait with merging one second - testing the changes in my expensify PR right now, there might be one problem left! |
Details
In this PR I added ranges to the onChange event:
onChange
event #412However there was a bug where when undoing using
ctrl + z
thestart
andbefore
values where incorrect.Additionally in this PR I did a cleanup and moved the code that calculates the replacement metrics to a function in the parseUtils.
Note: this PR contains changes from:
So we should merge that one first, or close it and instead just merge this PR.
Related Issues
Expensify/App#37896
Manual Tests
Linked PRs
Expensify/App#44285