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

Allow nested datamodels #484

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Conversation

Dakror
Copy link
Contributor

@Dakror Dakror commented Jul 1, 2023

When trying to split the UI logic into multiple components, i frequently encounter the issue of not being allowed to nest the respective data models. After simply trying out disabling the check and not seeing any immediate issues as well as no unit test failures, i decided to make this change. I changed the previous error to a info message to let the developer know.

I don't (yet) know of any bad side effects, and if more work would be needed to facilitate this change, let me know. I feel like this is very important to be able to do.

@mikke89
Copy link
Owner

mikke89 commented Jul 2, 2023

I don't remember if there was a specific reason for the constraint, or if it was just a conservative choice. In any case, I'm happy with making this change if I feel safe that it works.

One issue I see is that Element::SetDataModel won't work correctly if setting the parent data model last. We should look for other potential issues as well.

If we do eventually support this, I think we don't need the info message, since it is suggesting something non-standard or not supported. But it's nice to leave it in until we consider the feature stable.

@Dakror
Copy link
Contributor Author

Dakror commented Jul 2, 2023

It would probably suffice to check if the child in question has its own data model already and if so stop the descent of the parent model propagation. And yeah i agree about the info message. As always i will use this in my project to see if anything explodes :D

@Dakror Dakror changed the title Allow nested datamodels, but print info message about it happening Allow nested datamodels Jul 8, 2023
@mikke89 mikke89 merged commit 00c9541 into mikke89:master Jul 10, 2023
13 checks passed
@mikke89
Copy link
Owner

mikke89 commented Jul 10, 2023

Thanks for the PR, let us know if you encounter any issues with this.

@Dakror Dakror deleted the allow-nested-datamodel branch July 11, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants