fix: switch DnD to modern context API (was legacy) #1819

ehahn9 · 2020-11-29T17:04:05Z

DnD addon was relying on legacy context api which finally broke with React 17 (couldn't resize events, etc.).

Fixes #1795. Related to #1776.

ehahn9 · 2020-11-29T18:47:54Z

Hmmm... might still have remaining v17 compatibility issues due to DnD's use of findDOMNode (deprecated). Working on that too.

jquense · 2020-12-01T01:39:36Z

let me know when you want to merge 👍

ehahn9 · 2020-12-02T00:17:50Z

Thanks, @jquense -- the context switch was easy. The use of deprecated findDOMNode is harder (am working on that now).

The original DnD addon is looking like it needs some serious love (very non-DRY, odd js idioms throughout) -- I might take a crack some rejuvenation throughout, but I also want to see it working with React 17 asap.

I'll ping you when I have something useful to review!

ehahn9 · 2020-12-07T15:38:07Z

Hi @jquense... I just realized that the examples build on github seem to be of a different vintage than those on master? (for example, the page's dropdown is missing more recently-added examples).

I've been working on finishing up the DnD refresh and thought I had introduced a regression but noticed that the examples built from master have the same probs (!).

I'll dive into this but wondering if you had thoughts on why the examples page is out-of-date? (or maybe I'm missing something...)

jquense · 2021-01-04T14:15:01Z

@ehahn9 i just always forget to deploy new docs which is why they are out of date. we should make that automatic with the version release tho

Fixes #1795. Related to #1776.

ehahn9 · 2021-01-26T23:23:32Z

HI @jquense - I [finally, sorry] had a chance to work on this. Many changes:

Use modern react context API (was legacy)
Uses refs rather than findDOMNode throughout the addin (although alas there is a bunch in the main rbc code, thus preventing react strict mode...)
Moved the DnD readme to the top-level of the addin (was buried in code comment)
Storybook: updated to v6, respect allDay in events
Fixed event propagation bugs (seen only in react 17)
Fixed a number of bounding box and fencepost errors in WeekView
Changed the HOC to extend, rather than repeat propTypes and defaultProps

I had hoped to do a LOT more (css modules for scoped addon classnames, and major cleanup of original implementation which is difficult to maintain) -- maybe I'll get back to that at some point!

I'm still a newbie, so apologies if this PR needs more work (happy to try!).

yann-combarnous · 2021-02-21T08:51:25Z

@jquense, any chance you could review and possibly merge this PR? With DND resize broken in react 17, would be great to have it fixed.

ehahn9 · 2021-02-21T13:46:32Z

Hi @jquense - oh apologies - I'm new to this and I wasn't even aware that this was waiting on me. Sorry for asking for a lesson in OSS, but what's the protocol when one is reviewing their own PR? Do I just click squash + merge or should others be reviewing first/also? thx.

jquense · 2021-02-22T15:04:05Z

@ehahn9 generally you still want someone else to review it, but i've not been prompt on that sorry!

jquense · 2021-02-22T15:05:41Z

There is a lot of different fixes and changes here that make it a bit hard to review all at once. Generally its better to split these things into separate PR's, e.g. one for event propagation bugs, one for context changes, etc.

jquense

lots of great work, sorry i'm having a hard time following it!

jquense · 2021-02-22T15:06:02Z

examples/demos/dnd.js

@@ -55,7 +55,7 @@ class Dnd extends React.Component {

    const nextEvents = events.map(existingEvent => {
      return existingEvent.id == event.id
-        ? { ...existingEvent, start, end }
+        ? { ...existingEvent, start, end, allDay }


doesn't the existingEvent already have this?

The old story's moveEvent handler did't honor changes affecting allDay. This change purports to make it possible to demo moving events in and out of the allDay gutter - supported by DnD but previously not supported by the story as it ignored the new value for allDay.

I should have done this as a separate PR for sure! Newbie/learning.

jquense · 2021-02-22T15:06:43Z

src/addons/dragAndDrop/EventContainerWrapper.js

-    this.update(event, slotMetrics.getRange(currentSlot, end, false, true))
+    const { duration } = eventTimes(event, accessors)
+    let newEnd = dates.add(newSlot, duration, 'milliseconds')
+    this.update(event, slotMetrics.getRange(newSlot, newEnd, false, true))


I'm not sure i understand what the changes in this file are fixing? can you walk me through it?

Good catch. The problem I was addressing was that the addon would sometimes fail to update the event end on a move. This change always sets end = start + duration where start = newSlot (e.g. the target time slot at the end of the move).

jquense · 2021-02-22T15:09:37Z

src/addons/dragAndDrop/EventContainerWrapper.js


-EventContainerWrapper.propTypes = propTypes
+  render() {
+    return <div ref={this.ref}>{this.renderContent()}</div>


i don't think spliting renderContent out as a separate function adds much? maybe just have render().

On the another note, I think it might more more sense to pass the ref to children instead of adding a new DOM element

Absolutely right. I was having trouble forwarding the ref and took the coward's way out. I'll try to work on this.

jquense · 2021-02-22T15:10:31Z

src/addons/dragAndDrop/EventWrapper.js

    this.context.draggable.onBeginAction(this.props.event, 'resize', 'DOWN')
  }
  handleResizeLeft = e => {
    if (e.button !== 0) return
-    e.stopPropagation()


It seems like removing these would create bugs, not fix them? Can you walk me through why this is necessary?

Ok, this one is tricky. the Dnd addon uses rbc's selectable() mechanism to monitor mouse events before/during drags. selectable needs to see those events in order to work. The event wrapper stopped the propagation and so the selection logic was basically broken.

So first, I removed the stop propagation - that helped a lot.

But because the resize anchors are on top of the drag (move) anchor, clicking on a resize would also start a drag/move, which would abort the resize, etc.

So I added a check (see :42) to ignore this case:

// hack: because of the way the anchors are arranged in the DOM, resize // anchor events will bubble up to the move anchor listener. Don't start // move operations when we're on a resize anchor. const isResizeHandle = e.target.className.includes('rbc-addons-dnd-resize') if (!isResizeHandle) this.context.draggable.onBeginAction(this.props.event, 'move')

Now the million dollar question is why did this ever work? My only theory is that react 17 changed where the event listeners were placed and maybe that subtle changed the event propagation?

I would love to back all of this out, but it seemed to be required both in reading the code and trying it.

sounds like you got it all handled 👍

jquense · 2021-02-22T15:12:08Z

src/addons/dragAndDrop/WeekWrapper.js

@@ -79,42 +61,34 @@ class WeekWrapper extends React.Component {
    this.setState({ segment })
  }

-  handleMove = ({ x, y }, node, draggedEvent) => {
+  handleMove = (point, bounds, draggedEvent) => {


All the changes in this file are hard to follow (though i'm sure correct and needed). What is is going on here, and how should test that it works?

Purely idiomatic: all the other event handlers had the (point, bounds) signature (as does the similar code in EventContainerWrapper) so I was just conforming the idiom.

BTW, if you look closely, you'll see that WeekWrapper and EventContainerWrapper are in DIRE need of being DRY'ed out -- they basically do the same exact thing, with the main difference being the orientation of the event (horizontal or vertical). I spent a few days on that, but had to abandon due to time.

... oh, and the other changes (sorry, that's probably what you were asking about) - were all about making the date handling correct during drags - which had a number of probs, some of which might have been introduce with the (required) upgrade to date-arithmetic (which was out-of-date and causing some probs).

Happy to answer in more detail about these edits although my memory is fading a bit on this.

testing: I think the only way is via storybook/examples -- the code seems too intertwined IMHO for proper testing (definitely a good should a rewrite occur!)

fine to defer the testing if there isn't a good way to write a unit test. I was thinking of the common-ish case where we fix one of these date bounds issues and introduce another one, partly because the calculations have no tests asserting their current behavior so easy to break stuff!

ehahn9 · 2021-02-22T21:15:23Z

I'll wait to hear back from you (@jquense) before working further on this PR. Agreed, I really should have done this in more atomic pieces (and will next time!). Alas, I find the DnD addon to be super hard to work on (maybe even a rewrite) and so my energy was flagging during this... apologies.

jquense

I would rename the PR so semantic release doesn't call all of this a "fix", might be worth hand rebasing the commits into different messages and then doing a "merge" instead of a "squash" but up to you.

ehahn9 · 2021-03-02T15:48:58Z

Thanks @jquense - just to be sure I'm doing the right thing: are you suggesting I replace this PR with smaller, more focussed ones? (I presume there's a way to cancel this PR?) I'm happy to work on that! I know there's some time pressure for this due to v17 so I hope I'm not holding things up.

jquense · 2021-03-02T20:37:18Z

just to be sure I'm doing the right thing: are you suggesting I replace this PR with smaller, more focussed ones? (I presume there's a way to cancel this PR?)

not quite, the suggestion was to rebase the commits in the current PR to something like

fix: event propagation bugs
fix: drag and drop bounds issues
fix: update to modern context

etc, etc, and then merging instead of squash and merge so the commits stay intact on master. That said it all involves a bunch of Git-fu, that you may or may not be comfortable with, so fine to merge as is if you want

also upg date-arithmetic dep & some minor cleanup

ehahn9 · 2021-03-03T02:29:14Z

Just rebased all the commits and reworded - lmk if this looks okay to you -- then I'm happy to merge (merge commit, no squash)

jquense · 2021-03-03T02:36:56Z

very nice 👍

github-actions · 2021-03-03T03:00:58Z

🎉 This PR is included in version 0.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jquense · 2021-03-03T13:55:27Z

🎉 very nice

nlevchuk · 2021-03-05T06:01:30Z

@ehahn9 @jquense Thank you guys for your work

yann-combarnous · 2021-03-09T18:02:36Z

@ehahn9 , @jquense , thx for this! Any chance dnd could be made to work with touch events at some stage?

Dragging works but the initial event selection is very hard to get to work, takes generally a few seconds to manage selection.

ehahn9 · 2021-03-09T18:20:44Z

@yann-combarnous - although more knowledgeable folks might disagree, I suspect the addon might be at the limits of maintainability at this point :-(.

ps: I took a stab at rewriting it from scratch as a completely separate project using react-dnd, but ran into some architectural problems with resize animation.

fix: switch DnD to modern context API (was legacy)

6def209

Fixes #1795. Related to #1776.

jquense reviewed Feb 22, 2021

View reviewed changes

jquense approved these changes Feb 25, 2021

View reviewed changes

ehahn9 added 9 commits March 2, 2021 15:50

fix: replace findDOMNode with refs (react 17)

24f92fb

also upg date-arithmetic dep & some minor cleanup

chore(DnD): Move api info to README, minor cleanup

7c4e16d

feat(DnD): support to/from allDay events in demos

b067ad0

chore(deps): upgrade storybook

368ff2b

fix: mouse event propagation probs

759a232

fix: using wrong bounding box for hit testing

f670719

fix: DnD corner cases in allDay move/resize

5380fee

chore(DnD): DRY out common utility fn

c0aafc9

chore(DnD): HOC extend base propTypes/defaultProps

4574ab7

ehahn9 merged commit 611a0ce into jquense:master Mar 3, 2021

github-actions bot added the released label Mar 3, 2021

ehahn9 mentioned this pull request Mar 4, 2021

fix(DnD): selection in WeekView #1879

Merged

fix: switch DnD to modern context API (was legacy) #1819

fix: switch DnD to modern context API (was legacy) #1819

Conversation

ehahn9 commented Nov 29, 2020

ehahn9 commented Nov 29, 2020

jquense commented Dec 1, 2020

ehahn9 commented Dec 2, 2020 • edited Loading

ehahn9 commented Dec 7, 2020

jquense commented Jan 4, 2021

ehahn9 commented Jan 26, 2021

yann-combarnous commented Feb 21, 2021

ehahn9 commented Feb 21, 2021

jquense commented Feb 22, 2021

jquense commented Feb 22, 2021

jquense left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehahn9 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehahn9 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehahn9 commented Feb 22, 2021

jquense left a comment • edited Loading

Choose a reason for hiding this comment

ehahn9 commented Mar 2, 2021

jquense commented Mar 2, 2021

ehahn9 commented Mar 3, 2021

jquense commented Mar 3, 2021

github-actions bot commented Mar 3, 2021

jquense commented Mar 3, 2021

nlevchuk commented Mar 5, 2021

yann-combarnous commented Mar 9, 2021 • edited Loading

ehahn9 commented Mar 9, 2021

ehahn9 commented Dec 2, 2020 •

edited

Loading

ehahn9 Feb 22, 2021 •

edited

Loading

ehahn9 Feb 22, 2021 •

edited

Loading

jquense left a comment •

edited

Loading

yann-combarnous commented Mar 9, 2021 •

edited

Loading