Skip to content

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

Merged
Skalakid merged 3 commits intoExpensify:mainfrom
margelo:feat/add-range-info-in-on-change-event
Jul 9, 2024
Merged

feat: add count/before/start info to onChange event#412
Skalakid merged 3 commits intoExpensify:mainfrom
margelo:feat/add-range-info-in-on-change-event

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

@github-actions
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
Contributor

@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

@hannojg hannojg requested a review from Skalakid July 5, 2024 10:57
Copy link
Contributor

@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
Contributor

@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
Contributor

I haven't found any regressions 👍🏻

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