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

feat: add count/before/start info to onChange event #412

Merged

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented Jul 2, 2024

Details

As part of an effort to fix a long standing bug in expensify / react-native's text input I need to update the web implementation to work as the react-native's native change:

Goal: align with:

In this PR the onChange event is send with the range in which the new changes occurred. The range is encoded as three values:

  • start: The position in the new text where the changes started occuring
  • count: The length of the newly changed text
  • before: The length of the text before the changes.

Here are a few examples to better understand this:

  • Initial: "Hello"
  • Update to: "Hello**!**"
  • start: 5 (end of Hello)
  • count: 1 (the ! was added)
  • before: 0 (no text was replaced)

Second example:

  • Initial: "Hello"
  • Update to: "He0o"
  • start: 2 (after the e)
  • count: 1 (the 0 was added)
  • before: 2 (the text replaced ll had a length of 2)

This PR adds the same range info when we fire a onChange event.

Related Issues

Expensify/App#37896

Manual Tests

Still need to add (will do tmrw) - can I maybe get a code review first?

Linked PRs

Copy link

github-actions bot commented Jul 2, 2024

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

@hannojg
Copy link
Contributor Author

hannojg commented Jul 2, 2024

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

@hannojg
Copy link
Contributor Author

hannojg commented Jul 2, 2024

recheck

@hannojg hannojg marked this pull request as ready for review July 2, 2024 16:11
Copy link
Collaborator

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

Left some comments

src/MarkdownTextInput.web.tsx Outdated Show resolved Hide resolved
src/MarkdownTextInput.web.tsx Outdated Show resolved Hide resolved
src/MarkdownTextInput.web.tsx Outdated Show resolved Hide resolved
@hannojg hannojg requested a review from Skalakid July 5, 2024 10:57
Copy link
Collaborator

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

Code LGTM, I will run some tests now to check if there's no regressions 👀

@BartoszGrajdek
Copy link
Collaborator

I haven't found any regressions 👍🏻

@Skalakid Skalakid merged commit 3859831 into Expensify:main Jul 9, 2024
5 checks passed
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