react-number-format icon indicating copy to clipboard operation
react-number-format copied to clipboard

Published 20 hours ago • verified publisher icon s-yadav

Test upgrade

Open s-yadav opened this issue 1 year ago • 6 comments

Describe the issue/change

Add CodeSandbox link to illustrate the issue (If applicable)

Describe specs for failing cases if this is an issue (If applicable)

Describe the changes proposed/implemented in this PR

Link Github issue if this PR solved an existing issue

Example usage (If applicable)

Screenshot (If applicable)

Please check which browsers were used for testing

  • [ ] Chrome
  • [ ] Chrome (Android)
  • [ ] Safari (OSX)
  • [ ] Safari (iOS)
  • [ ] Firefox
  • [ ] Firefox (Android)

s-yadav avatar Jul 22 '23 13:07 s-yadav

The changes from #763 are not ready to be merged into main yet. I think it would be better to close this PR and create a new one when some of the other ongoing work in the dev branch has been completed.

aashu16 avatar Jul 25 '23 13:07 aashu16

A couple of small changes left to make - then this should be ready to merge.

@s-yadav take a look when you can and let me know if you have any further comments.

I would rather create a new release branch once the remaining changes have been made and then merge that into main. So, I am going to close this PR now. Please feel free to reopen if you would rather merge this directly.

aashu16 avatar Mar 17 '24 18:03 aashu16

@aashu16, Aaah better to use the same PR, so I can just check on changes that has been done after the last review. Or else I have to go through all the changes again. Not sure if we reopen github will allow seeing the changes made afterwards.

Anyways other option is that I can check from commit ranges. But Yes please don't squash all the commits until the review is finished.

s-yadav avatar Mar 18 '24 05:03 s-yadav

@aashu16, Aaah better to use the same PR, so I can just check on changes that has been done after the last review. Or else I have to go through all the changes again. Not sure if we reopen github will allow seeing the changes made afterwards.

Sure, no problem. I will leave this open for now and keep making changes. You should still be able to see your comments and my responses. I marked them as resolved, but feel free to undo that.

Anyways other option is that I can check from commit ranges. But Yes please don't squash all the commits until the review is finished.

Not planning on squashing anything 😄

aashu16 avatar Mar 18 '24 15:03 aashu16

@aashu16 Merged master and fixed some broken tests which was incorrect on master as well. Also, I see couple of eslint error for unused variables in testing files, maybe you can remove those as well.

s-yadav avatar Mar 24 '24 07:03 s-yadav

@s-yadav, I think this is just about ready to merge now. Let me know if you have any comments regarding the recent commits.

(Please don't merge yet.)

aashu16 avatar Apr 06 '24 01:04 aashu16

Hey @aashu16, all changes look good. I think let's merge this as there are couple of bug fixes I was planning to do, and better to do with new tests. If there are any changes required we can do that on separate pr.

Also, was making some changes to the core logic so wanted to do it on this branch only, seems like there was test for it, but earlier it was just giving false positives. In this branch, it was marked for todo. Have made that, plus fixed all the todo tests, and also some changes around test_util to support my changes.

This are my changes. https://github.com/s-yadav/react-number-format/pull/779/files/91e8fd263a815f00d15f542a02dbad23c526ed6e..ca97625a2296f6fe39d33e9e4736e1cb7243cac2

s-yadav avatar May 12 '24 18:05 s-yadav