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

'remove' event handling inconsistent with other events #13

Closed
wellcaffeinated opened this Issue Aug 7, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@wellcaffeinated
Contributor

wellcaffeinated commented Aug 7, 2012

Remove event handling is inconsistent with the other events. No key name is provided to the event handler for remove actions... making the idea of remove event handling unfortunately useless.

Module.set('key', 'value'); // fires (create, create:key, mutate, mutate:key, change, change:key) events with parameters
Module.set('key', 'othervalue'); // fires (update, update:key, mutate, mutate:key, change, change:key) events with parameters
Module.remove('key'); // fires (remove, change) events without any parameters!

Request that this is changed to:

Module.remove('key'); // to fire (remove, remove:key, mutate, mutate:key, change, change:key) events with appropriate parameters
// such that
Module.on({
    'change': function( v ){
        // v === 'key'
    },
    'change:key': function( v ){
        // v === LAST VALUE
    },
    'mutate': function( v ){
        // v.oldValue === LAST VALUE
        // v.newValue === undefined
        // v.key === 'key'
    },
    'mutate:key': function( v ){
        // v.oldValue === LAST VALUE
        // v.newValue === undefined
        // v.key === 'key'
    },
    'remove': function( v ){
        // v === 'key'
    },
    'remove:key': function( v ){
        // v === LAST VALUE
    }
});
@wellcaffeinated

This comment has been minimized.

Show comment
Hide comment
@wellcaffeinated

wellcaffeinated Aug 8, 2012

Contributor

Submitted pull request to resolve this issue: #14

Contributor

wellcaffeinated commented Aug 8, 2012

Submitted pull request to resolve this issue: #14

@hay

This comment has been minimized.

Show comment
Hide comment
@hay

hay Aug 10, 2012

Owner

I agree, remove() needs to throw the same events as set(). However, i think the implementation is a little too complicated, with having an extra parameter to setAttribute(). It makes the code less readable. If you could provide an implementation that is cleaner i would happily include it in the next release of Stapes.

Owner

hay commented Aug 10, 2012

I agree, remove() needs to throw the same events as set(). However, i think the implementation is a little too complicated, with having an extra parameter to setAttribute(). It makes the code less readable. If you could provide an implementation that is cleaner i would happily include it in the next release of Stapes.

@hay hay closed this Aug 10, 2012

@wellcaffeinated

This comment has been minimized.

Show comment
Hide comment
@wellcaffeinated

wellcaffeinated Aug 10, 2012

Contributor

What would a cleaner version look like? Would you be more agreeable to a .removeAttribute() method?

Contributor

wellcaffeinated commented Aug 10, 2012

What would a cleaner version look like? Would you be more agreeable to a .removeAttribute() method?

@hay

This comment has been minimized.

Show comment
Hide comment
@hay

hay Aug 11, 2012

Owner

Yes, i think it would make more sense to have both a remove and setAttribute method.

Owner

hay commented Aug 11, 2012

Yes, i think it would make more sense to have both a remove and setAttribute method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment