DnD support for custom event components

[ehahn9]

[second attempt, hopefully better PR]

I've been trying to use the drag and drop addon with a custom Event component. Alas, the addon needs to use its own ResizeEvent component and thus overwrites mine.

I have no idea if this PR is useful to anyone else, but I thought I'd submit it just in case in inspires someone more experienced than me to work on this further :-)

This PR changes the behavior to use a HOC (withEventResizability()) to compose a new component which effectively wraps any caller-supplied Event component. (To mimic the base Calendar behavior it wraps a new TitleComponent as the default).

I also tried to DRY up the original ResizeEvent and ResizeMonthlyEvent into a single wrapper which takes a direction (horizontal or vertical) to determine where the anchors go, etc.

And I changed a bunch of other gratuitous stuff like adding more docs, using static class props, etc. I also added two new storybooks: one showing drag+resize, the other drag+resize with a custom event component.

Feel free to discard/close this if it isn't useful, but also feel free to put me to work on edits/updates - I'll give it my best shot, time-permitting.

(ps: I know addons aren't really part of react-big-calendar - apologies if I've put this issue in the wrong place).[

[jquense]

hey sorry i'm not sure why the current api doesn't already work? Addons shouldn't be specifying Event components at all, the DnD addon (at least used to) just use the EventWrapper component, if it's not it should go back to doing that, vs something more complicate like this. The Wrapper components were specifically added for these use cases.

[jquense]

(and thanks for another PR btw)

[ehahn9]

Yeah, alas the current version of dnd in addons indeed does smash the event component. It does this to make one with drag anchors, etc. You raise a great point - why can't this be done 100% in the wrapper!

It looks like the original draggable implementation used a custom EventWrapper but the resizable (recent?) addition added the custom ResizableEvent.

I'll take a look at using the wrapper for both - it would make things a LOT cleaner (thanks).

ps: might be some other stuff in this PR (like the storybooks, docs and a bit of minor cleanup) which might be helpful. Feel free to close this if you prefer, or I'll push updates as soon as I have a moment to work on this. Thanks for your help and suggestions.

[jquense]

I do think it's fine for addons to provide default components, but the Wrapper should inject things like resize handles either as markup or as a callback or something, react dnd already has these patterns. That way if someone wants to customize the Event component they have the extra props needed to add the resize handles, otherwise we can use the default in the addon (so we don't polute the core app with assumptions of addons)

[akeroyd]

Moving back to the wrappers should also fix this bug: #629

[ehahn9]

I'm working on this... I've removed the need for a custom Event component (does everything in the EventWrapper... thanks @jquense for that excellent lead!).

Turns out that the dnd addon ended up getting quite a bit of reworking - maybe I've been overzealous... it is going to be a pretty big diff after all. I'm not sure of the oss etiquette here and certainly don't want to submit a PR which oversteps acceptable bounds (!). Even so, there are a lot of DnD corner cases which neither the current, nor updated code handles gracefully, so at best, this is an incremental "improvement". I'll submit the PR when I think it's presentable - would love feedback from anyone so inclined! No offense will be taken if the PR is too divergent!

[arecvlohe]

I like these changes. Consolidating the resize functionality into one component works for me. I think that is what we initially wanted to do but did not get around to it. Thanks for the PR. I think it's a much needed improvement.

[ehahn9]

I think there's still a lot of work I need to do on this... I guess I'd like a bit more time (and another update) in the coming week if we can hold off? Since I issued the PR, I've been working on a few biggish things to further streamline:

• implemented all of the drag & resize in the EventWrapper - no more withEventResizability hoc (which was sort of a hackish attempt on my part, honestly)
• reworked how the anchors are handled in the render and css (mostly unifying the n-s and e-w drag handles, etc.)
• normalizing the drag item payloads and handling in the various drop targets

I'm new at react-dnd so am moving rather slowly - apologies!

BTW:

Along the way, I've been perplexed by the choice to have onEventDrop and onEventResize have such different callback signatures... Is there a reason to have the "drop" parameter always first on onEventResize? Seems like it is a bit confusing (and unused?). I realize we don't want to break compatibility, but would love to hear the rationale for keeping it long term?

[arecvlohe]

@ehahn9 When the idea was first implemented the thought was to have a drop and resize be handled differently but in retrospect it doesn't matter because we have the callback functions to handle that anyway. Take the code with a grain of salt since it was worked on and off for a few months before it was implemented. If the API is updated as you propose, ie removing the eventType, it will be very important to bring attention to this as many have wanted, and I imagine, implemented this feature based on the current version.

There is no rush to get this working right way. The initial implementation was just to get something out there for others to use and then tweak it more going forward. At least that was my approach to the whole thing. When you are ready to have this reviewed just give a holler and I will take another look. The process is to have at least 2 contributors approve before merging.

The fact that you are working on improving this is great and I appreciate the effort!

[jquense]

This is looking good! For discussion, a downside with the current approach is that a user can't customize the resize handles in EventComponent if they wanted too. The upside is that the component "just works" nicely out of the box, even with a custom EventComponent.

I'm not sure if that tradeoff is the right one but maybe that's something we can change if folks complain about it? This is definitely more flexible than what we have now so either way it's an improvement

[jquense]

this neat, one issue here tho is that this creates a new class per render, which is not cheap, for a lot of reasons. THe big one being React can never reuse the the markup because the render -> render the "type" is actually different

[jquense]

I wouldn't use a HOC here, instead I'd move the withEventResizability logic directly into the event wrapper component, or if there is value in splitting it out (does anything else use the HOC?) Use it on the wrapper component wrapping props.children instead, leaving EventComponent alone until it's being rendered. The other thing that avoids, btw, is the fact that a user can specify an EventComponent per view like:

components = {
  event: DefaultEventComponent,
  month: { event: MonthEventComponent },
  week: {  event: WeekEventComponent },
}

[jquense]

I just noticed you said:

implemented all of the drag & resize in the EventWrapper - no more withEventResizability

above, Sorry ignore those bits of the comments :)

[ehahn9]

As promised, here's the updated PR:

• everything is handled in EventWrapper (no more custom Event component!). The new wrapper handles both normal and allDay-style anchors with common implementation (no more ResizableMonthEvent etc.)
• onEventDrop now includes allDay property so you know if an event was dropped in the day header area (start and end unchanged for backward compat)
• better handling of anchors in multi-day events and events which continue beyond the current display range
• used static class props for propTypes etc. where appropriate
• added support for case where the Calendar has step !== 30
• minor css changes, including showing the drag anchor on .rbc-event:hover - may be too aggressive?
• 3 more stories to show various cases of DnD

Delighted for any inputs/comments - so sorry it ended up being such a big PR - let me know if I've bitten off too much!

[arecvlohe]

I have a question on resizing the event in month view when it is not an all day event.

[arecvlohe]

Wouldn't resizing an event, that is not all day, in month view extend the event to be more than one day long? Meaning it would extend the event to the next day at the same end time as it was previous. Am I missing something?

[ehahn9]

@arecvlohe you're absolutely right - your suggested behavior is perfectly reasonable, too. In fact it matches how drop (as opposed to resize) works when dropping a non-allDay event on the date header: it retains the times but changes the date.

Great catch. I'll change the behavior and re-push when I have a moment to work on this!

[arecvlohe]

This commented out stuff has been here for a while and I am wondering if it's still needed. Thoughts @jquense?

[jquense]

sorry about that, its my own personal reminder for how to structure the react-dnd stuff so that the drop areas align with the shap of the even vs just where the mouse is over (which i think is more intuitive feeling) I left it there to save my place, but haven't had the time to explore it any further. We can remove it (git has is saved obviously) unless someone wants to pick it up ;)

[ehahn9]

I sure would love to see the drop target be based on the mouse-relative location, not the top of the event. I agree that the current behavior isn't the best user experience. I ran out of energy (and competence :-)) on this so didn't tackle it.

[ehahn9]

@arecvlohe: changed drop behavior to preserve times when dropping on days (some tricky corner cases, but all happiness in the end!). Also clean up docs to match.

[arecvlohe]

Awesome!

[seeyoumr]

Hi @ehahn9 . Can you provide an example? Both resizable and custom event components are supported. very thankful !

[ehahn9]

I'll try to work on an example in the coming week. The existing DnD example is easily extended.

In the meantime, there's a fair bit o' documentation in addons/dragAndDrop/withDragAndDrop.js and a few new storybook examples.

[jquense]

Lgtm,but I haven't had a chance to try it .I defer to the rest of y'all on thus :)

[KerenChandran]

Hey what's the status on this PR? Would like to know if this is stable to be merged to master or if it requires more work. Awesome work btw! 👍

[arecvlohe]

@ehahn9 When you think the PR is ready go ahead and merge.

[ehahn9]

Hi @arecvlohe - embassingly, this is my first open source PR so I'm a bit fuzzy on process - I apologize. Can you help me understand your request? I guess I don't feel confident enough to "own" the merge to the master, but open to learning. (I'm guessing it isn't as simple just git merge...)

[arecvlohe]

@ehahn9 A PR needs 2 approvals from collaborators for it to be accepted into the code base, which you have on this.

Since the commit is pretty large I don't think anyone else wants to merge in something not knowing if it is complete or not. For that reason, you have ownership over the commit. I think the only other thing you need to do is merge master into your branch and it should be good to go. If you think it passes all your checks feel free to merge by clicking the green button, squash and merge. If you are still hesitant, just reach out and I can do it. But I want to make sure it's ready before I do.

[arecvlohe]

I think it would help to write up a contributing doc for the repo to clarify some of these things.

[ehahn9]

Thanks, @arecvlohe -- looks like the merge is ready. I'm happy to do the merge -- best to merge or merge-squash? Sorry for all the newbie q's...

[jquense]

squash and merge is preferred here yeah 👍