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

test: coverage for trait changeProp not set #6101

Merged
merged 2 commits into from
Sep 2, 2024
Merged

Conversation

danstarns
Copy link
Member

This PR is to add coverage and to start a discussion around changeProp and what should happen if the value is set or not.

Related Comments:

@danstarns
Copy link
Member Author

@artf would love your guidance here on changeProp how we work with it with data sources and how we should test it.

@danstarns danstarns mentioned this pull request Aug 30, 2024
Copy link
Member

@artf artf left a comment

Choose a reason for hiding this comment

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

I'd say the initial reason behind the trait.value is like the default one.
So if it's defined it should be applied to the component model (like you already did) but at the end the trait should always read/write from/to the component model.

The trait.value right now it's an intermediate state, when you do trait.setValue, this should update the component model and the UI of the trait.

From Trait.setTarget you can also notice how we subscribe to the component model changes in order to update trait.value.
That means if you update the property directly from the component, that will remove the dataSource binding in the trait. Does it make sense? I'm not 100% sure, but in my opinion it does.

IMHO it's fine as it is right now, but feel free to post here your concerns

@danstarns
Copy link
Member Author

That means if you update the property directly from the component, that will remove the dataSource binding in the trait. Does it make sense? I'm not 100% sure, but in my opinion it does.

We do store the data variables in an adjacent property called attributes-data-variable see in dev:

So I believe we should have synchronization even when someone updates the value directly, as the getters will look for the attributes-data-variable: KEY and then resolve the data var.

@danstarns danstarns merged commit 9d90e00 into dev Sep 2, 2024
2 checks passed
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