Skip to content

Conversation

@wclr
Copy link
Contributor

@wclr wclr commented Sep 28, 2020

@cycle/react currenlty replaces react handler on the element with its own event handler, this probably should be changed and the existing handler should be respected and executed.

The case for this: sometimes react expects that the event handler to return a value (for example if to pass the event or something like this). So this PR checks if handler exists and executes it after cycle event handler.

Also added prettier config. If ok with this PR, probably need to check and set new formatting of other files as well.

@staltz

Copy link
Member

@staltz staltz left a comment

Choose a reason for hiding this comment

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

Good PR, I think this change makes sense. I have one request, written inline.

Note, ideally the addition of prettier would be a separate PR (or at least separate commit), it's not good for codebase maintenance (e.g. using git blame) to have unrelated changes all together in one commit, but I don't mind merging it like this right now. So, no action needed from you, I'm just informing what's preferable for the next times.

@wclr wclr force-pushed the respect-react-event-handler branch from 9068d46 to c201239 Compare September 29, 2020 06:43
@wclr
Copy link
Contributor Author

wclr commented Sep 29, 2020

Made PR with formatting #14 and then tried to rebase this PR, not sure if I did right with this PR.

@staltz
Copy link
Member

staltz commented Sep 29, 2020

The if change looks good. Now we just need this PR rebased or conflicts resolved, to merge. If you have difficulties rebasing, you can also open another branch/PR and git cherry-pick the commit.

@staltz
Copy link
Member

staltz commented Sep 29, 2020

I suppose you're done and this is ready to merge? @whitecolor

@wclr
Copy link
Contributor Author

wclr commented Sep 29, 2020

I believe yes, if not count a mess with commits.

@staltz staltz merged commit b9d77e6 into cyclejs:master Sep 29, 2020
@staltz
Copy link
Member

staltz commented Sep 29, 2020

Thanks!

@staltz
Copy link
Member

staltz commented Sep 29, 2020

Release 2.9.0

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