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

Multiple properties in nested key loses data on changeset.set #398

Closed
josemarluedke opened this issue Jan 11, 2020 · 5 comments · Fixed by #404
Closed

Multiple properties in nested key loses data on changeset.set #398

josemarluedke opened this issue Jan 11, 2020 · 5 comments · Fixed by #404
Labels

Comments

@josemarluedke
Copy link
Contributor

Weird behavior when a model has more than one property inside of a nested key. It seems that the data is lost in the changeset for a moment.

In the example, we have the following model:

  model = {
    email: 'hi@gmail.com',
    user: {
      firstName: null,
      lastName: null
    },
    notifications: {
      email: true,
      sms: true
    }
  }

It's probably easier to see the issue in the gifs below.

Version

3.0.0-beta.7

Test Case

https://github.com/josemarluedke/--changeset-glimmer (Commit: https://github.com/josemarluedke/--changeset-glimmer/commit/10b7be96ea362378c0926b08cb504e705a1c4316)

Steps to reproduce

Close repo, run app, change "user.firstName", change "user.lastName", Sumit form, change "user.firstName" again. You will see "user.lastName" will lose it's data.
See gifs below:

Case 1, changing one checkbox, makes the second checkbox lose its data.

Screen Recording 2020-01-10 at 09 45 PM

Case 2, changing the value of input after the data is saved in the model makes the other input to lose its data.

Screen Recording 2020-01-10 at 09 44 PM

Expected Behavior

Both properties in the same nested key should have its data intact.

@snewcomer Sorry for these issues, it's hard to explain some of them. Let me know if you have any questions.

@snewcomer
Copy link
Collaborator

@josemarluedke Thank you thank you! Am I on the right path with the test I added in the below PR? What am I missing from your reproduction?

https://github.com/validated-changeset/validated-changeset/pull/9/files

@josemarluedke
Copy link
Contributor Author

@snewcomer Commented in that PR. I suggested adding a new case where only one property is set at a time. The tests seem to pass there fine, so probably the issue is in the Ember integration. Although, it doesn't seem you are using that lib here yet.

@snewcomer
Copy link
Collaborator

@josemarluedke Alright I had the chance to checkout your fork. I think the issue was the README. the value attribute on the input should reference the model. B/c live value on the input doesn't reflect back the attribute, this would be recommended. What do you think?

b9349c4

@josemarluedke
Copy link
Contributor Author

Repost of my comment in (#401 (comment)):

I don't think changing the readme to advise folks to use the model in forms is correct. I believe forms should always reflect the in-flight model. This way, we can revert changes and have forms reflect it without any issues (for example). Also, If you have a form in a tab layout for example, and you hide half of the form then go back to that, the form would show the incorrect data, the user would think they have lost their changes.

@snewcomer
Copy link
Collaborator

You are 100% correct. The model.value would be more appropriate based on the value property doesn't reflect on the value attribute. But for rollback and the example you provided to work, you are right.

Really appreciate the input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants