Added onDayClick event - Githubissues

dptoot / react-event-calendar

A React calendar component to display events

131 stars 55 forks source link

Added onDayClick event #23

Closed calvinclaus closed 8 years ago

calvinclaus commented 8 years ago

This is a WIP for #21.

I added an onClick(target, day) property to EventCalendar that is passed down to CalendarDay. It is called when a user clicks on any day in the calendar. If the user clicks on an event directly, the onClick function is not called, as I added a stopPropagation for the onEventClick event in CalendarEvent.

WIP I couldn't figure out an easy way to pass a meaningful target property to onClick. At the moment I am just passing the string 'day'. I saw that in CalendarEvent you are passing this, but we cannot do that in CalendarDay as long as its a functional component. Please let me know how you would like this issue to be resolved.

Other In case you like this addition, I am happy to make the changes to README.md, before this is merged.

idolize commented 8 years ago

Overall looks pretty good, although the indentation seems a bit odd to me personally.

calvinclaus commented 8 years ago

What do you think about the name onClick. After I implemented it, I thought onDayClick would fit better? Thoughts?

calvinclaus commented 8 years ago

Summing up:

Rename to onDayClick
Fix .bind(...)() nonesense
Make CalendarDay a class and pass this as target
Add an eventData prop to CalendarDay and pass this eventData to onDayClick [Postponed, waiting for @dptoot]

So we would end up with: onDayClick(target, eventData, day)

idolize commented 8 years ago

Good point - onClick is probably too vague.

idolize commented 8 years ago

eventData may or may not be necessary actually because the click is happening on the background of a day and not a specific event. Personally I don't care much about that, but maybe @dptoot has a suggestion

calvinclaus commented 8 years ago

Okay. So far so good 🔥 . Let me know if the changes work for you.

dptoot commented 8 years ago

Can't a man watch the euros in peace around here? :) sorry for the silence guys. I will review this tomorrow and push it out

idolize commented 8 years ago

Haha thanks @dptoot

dptoot commented 8 years ago

Looks pretty good overall. If you can make the changes I suggested and update docs and the demo I would appreciate it. Nice work.

calvinclaus commented 8 years ago

I think this should be ready. Let me know if you want me to rebase this to a sinlge commit.

dptoot commented 8 years ago

Looks good guys. Nice addition.

dptoot commented 8 years ago

Released in version 0.3.0

idolize commented 8 years ago

Thanks @calvinclaus!