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

fix(moveFieldState): get change/blur/focus callbacks from oldState #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SeqviriouM
Copy link

@SeqviriouM SeqviriouM commented May 22, 2020

Previously in function moveFieldState callbacks change/blur/focus is requested by new destKey. But I think it will be more correct to save previous callbacks. For example, when I remove second value from array I will expect it will also remove second callback with it. So I have tuned a bit test cases. It should fix issues #49 #51.

What do you think about this changes?

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #54   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          159       159           
  Branches        34        34           
=========================================
  Hits           159       159           
Impacted Files Coverage Δ
src/swap.js 100.00% <ø> (ø)
src/insert.js 100.00% <100.00%> (ø)
src/moveFieldState.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e27b5c...f098813. Read the comment docs.

@SeqviriouM
Copy link
Author

SeqviriouM commented May 27, 2020

@erikras can you check, please?

Comment on lines 33 to +34
const incrementedKey = `${name}[${fieldIndex + 1}]${tokens[2]}`
moveFieldState(state, backup[key], incrementedKey)
moveFieldState(state, backup.fields[key], incrementedKey, backup)
Copy link

Choose a reason for hiding this comment

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

@SeqviriouM Found your PR while was trying to resolve issues with insert.
Tested this code in app, where I have issues with insert and it didn't work well. Deeply nested fields handlers are messed up, so changing value in one field changes value in another one(I think because of moving handlers). What helped me is just:

delete state.fields[key]

So all fields after inserted one get default state and handlers.

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.

2 participants