Add draggable() feature to p5.Element #6381
Conversation
|
Thanks @ffd8! Looks good to me. Will merge it after @limzykenneth approves it. |
src/dom/dom.js
Outdated
| pos4 = 0; | ||
| if (elmnt !== this.elt && elmnt.elt !== this.elt) { | ||
| elmnt = elmnt.elt; | ||
| this.elt.onmousedown = dragMouseDown; |
There was a problem hiding this comment.
How do you feel about using addEventListener to avoid overwriting the onmousedown property if something else on a page has set it? Probably more for when we do it to document than to an element, but it might have less unintended side effects.
There was a problem hiding this comment.
@davepagurek Aha - good catch! especially since the implementation was also using document.onmouseup and document.onmousemove, which very well could run into issues with other interactions on the page. Here's a new example with listeners which runs great = will update code and PR now...
src/dom/dom.js
Outdated
| e = e || window.event; | ||
| pos1 = pos3 - parseInt(e.clientX); | ||
| pos2 = pos4 - parseInt(e.clientY); | ||
| pos3 = parseInt(e.clientX); |
There was a problem hiding this comment.
Maybe we can give these some more descriptive names so future maintainers will be able to read it more easily. Maybe prevX/Y and deltaX/Y or differenceX/Y?
There was a problem hiding this comment.
Good idea – was paying homage to where the code came from, but makes sense to update.
src/dom/dom.js
Outdated
| * } | ||
| * </code></div> | ||
| */ | ||
| p5.Element.prototype.draggable = function (elmnt = this.elt) { |
There was a problem hiding this comment.
It might simplify the if statement below if the default value is this -- that way the input is always a p5.Element.
There was a problem hiding this comment.
Took a while to figure out/undo my weird if logic – but solved it!
src/dom/dom.js
Outdated
| @@ -2421,6 +2421,93 @@ p5.Element.prototype.drop = function (callback, fxn) { | |||
| return this; | |||
| }; | |||
|
|
|||
| /** | |||
| * Turns p5.Element into a draggable item with the mouse. If argument is given, it will drag a different p5.Element (parent container) instead, ie. for a GUI title bar. | |||
There was a problem hiding this comment.
Maybe we can describe the example's behaviour in a bit more detail here, something like: this lets you drag the title bar of a panel to move the whole panel around
src/dom/dom.js
Outdated
| this.elt.addEventListener('mousedown', dragMouseDown, false); | ||
| this.elt.style.cursor = 'move'; | ||
| } else { | ||
| elmnt.addEventListener('mousedown', dragMouseDown, false); |
There was a problem hiding this comment.
I was trying this out on my phone and noticed (at least in the p5 editor) that dragging doesn't work. do you think we need to also add a fallback pointer event listener for mobile support?
There was a problem hiding this comment.
Good point – I was only testing on desktop, nevertheless got it working for touch as well now!
|
Hi, just following up on this PR. @ffd8, could you take a look at the comments from @davepagurek? @limzykenneth, let us know if you have any comments. Thanks. |
|
@Qianqianye Hihi – this fell to the back burner since there wasn't time to make requested changes – nevertheless, had a close look and implemented all suggestions above. Hope it's now ready to make into the next release! Perhaps not the place, but is there a timeline for a minor release to address introduced with 1.8.0? Personal to me and my lil world: |
|
@ffd8 The plan is to do a patch release next Tuesday, we'll need to review any critical fixes need merging before then as well. |
|
Thanks @ffd8 for working on it, and @limzykenneth @davepagurek for reviewing. With this new feature added, the next release will be a minor version, which is 1.9.0. |
Resolves #6248
Changes:
As discussed, adds a very simple 'p5.js' way to make DOM elements draggable without the need for additional libraries.
This works for both the element itself or using elm to move a parent container.
Basic example added to documentation) here:
https://editor.p5js.org/ffd8/sketches/c-gRg5Rv-
PR Checklist
npm run lintpasses