Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed ordering of setters so that user event handlers override defaults #37

Merged
merged 1 commit into from
Jan 3, 2019

Conversation

davezuch
Copy link
Member

@davezuch davezuch commented Jan 3, 2019

What does this pull request do?

Turns out the last IProp for an event handler wins. The reason we thought otherwise is that our lovely appendIProps function in Ocelot reverses the order of the props. But anyone not using Ocelot, or at least appendIProps, will not be able to override the default props here.

Other Notes:

I have a corresponding branch in Ocelot that fixes appendIProps. Once this PR is approved and a new release tagged, I'll make a PR there as well.

@davezuch davezuch requested a review from crcornwell January 3, 2019 20:25
@@ -118,7 +118,7 @@ setItemProps
. Int
-> Array (HP.IProp (ItemProps p) (Query o item Unit))
-> Array (HP.IProp (ItemProps p) (Query o item Unit))
setItemProps index = flip (<>)
setItemProps index = (<>)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could just be append

Copy link
Member Author

Choose a reason for hiding this comment

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

True...but actually more verbose lol

@thomashoneyman
Copy link
Collaborator

I knew something was up!

@thomashoneyman
Copy link
Collaborator

thomashoneyman commented Jan 3, 2019

@linearray I'm assuming this is related to your issue in Slack. Glad @davezuch saw the cause of the problem.

@linearray
Copy link

Thank you for the quick fix!

@davezuch davezuch merged commit c646a55 into master Jan 3, 2019
@davezuch davezuch deleted the setters-order branch January 3, 2019 21:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants