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

sync[attribute] Methods not working #146

Closed
ethib137 opened this issue Aug 3, 2016 · 2 comments
Closed

sync[attribute] Methods not working #146

ethib137 opened this issue Aug 3, 2016 · 2 comments

Comments

@ethib137
Copy link

ethib137 commented Aug 3, 2016

import Component, {Config} from 'metal-jsx';

class TestKit extends Component {
    created() {
        this.handleClick_ = this.handleClick_.bind(this);
    }

    handleClick_() {
        this.state.visible_ = !this.state.visible_;
    }

    syncVisible_(newVal) {
        console.log('newVal:', newVal);
    }

    render() {
        return (
            <div>
                <a href="javascript:;" onClick={this.handleClick_}>{'Toggle'}</a>

                {this.state.visible_ &&
                    <div>
                        {'Component Content'}
                    </div>
                }
            </div>
        );
    }
}

TestKit.STATE = {
    visible_: Config.value(true)
};

export default TestKit;

I would expect syncVisible_ to be called anytime the toggle is clicked. Currently it's never being called.

Let me know if you have any questions.

Thanks.

@mairatma
Copy link
Contributor

mairatma commented Aug 3, 2016

Now that jsx components data is split between props and state, it's harder to keep the feature of sync methods working for all data. For example, let's say you had a visible prop as well, then when would you expect syncVisible to be called? Just for the one in props, just for the one in state, or for both of them (with a flag maybe)?

When I did the separation I ended up choosing to keep the sync methods working just for props, since you already have more control about when state changes, as it only happens internally. You can see that it would work if this was using a prop instead.

Do you think this would be better for jsx components if it worked differently? If so, let me know and we can change so it has the best behavior for jsx use case.

@ethib137
Copy link
Author

ethib137 commented Aug 3, 2016

Hey Maira,

That seems reasonable to only include sync for props now. I just didn't realize that was intended. Thanks for the quick response.

I'll go ahead and close this issue.

@ethib137 ethib137 closed this as completed Aug 3, 2016
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

No branches or pull requests

2 participants