Drag drop console cells into notebook #5585

Madhu94 · 2018-11-04T12:58:07Z

ian-r-rose · 2018-11-12T17:00:06Z

Thanks for tackling this @Madhu94! Is it ready for review yet? I am trying it out and having a hard time getting the console cell drag to work.

Madhu94 · 2018-11-21T21:54:52Z

Sorry, I didn't notice the above update. @ian-r-rose Could you try it out now? I wouldn't say it is ready for review - some tests were failing and I couldn't figure out if I broke it. I will check those now

ian-r-rose · 2018-11-30T23:02:35Z

I'd say don't worry about the tests too much unless they look specifically related to your functionality -- we are still working out some problems with the test suite. Better to get the functionality where you want it, and we can address the tests later.

I was just playing with this, and I am still getting some errors when I try to drag the cells:

ReferenceError: event is not defined[Learn More] cellDragUtils.js:76
detectTargetArea
cellDragUtils.js:76
_evtMouseDown
widget.js:1160
handleEvent
widget.js:849

Madhu94 · 2018-12-01T10:10:26Z

This was the error I got too, when you said earlier that it didn't work. I had fixed it later. I had squashed the fix commit into the original one and force pushed. Can you check if you pulled the latest changes from this branch? (you probably have, just confirming).

Madhu94 · 2018-12-01T10:11:32Z

The test cases do not seem related, seems like a path resolution issue ?

ian-r-rose · 2018-12-04T18:02:54Z

Ah, you are absolutely right @Madhu94, my apologies! I messed up the fetch-after-force-push.

ian-r-rose

Thanks @Madhu94, this is really great! I have a couple of minor comments, but overall this is in good shape.

It also looks like there is a conflict in the yarn.lock, so a rebase would be useful.

ian-r-rose · 2018-12-04T18:46:22Z

packages/cells/src/index.ts

@@ -5,6 +5,7 @@

 import '../style/index.css';

+export {default as CellDragUtils, ICellTargetArea} from './cellDragUtils';


I found this construction a bit confusing. Can we create a CellDragUtils namespace directly in the cellDragUtils.ts file?

Do we intend to continue usage of namespaces? I've heard it isn't recommended, correct me if I'm wrong. E.g http://artsy.github.io/blog/2017/11/27/Babel-7-and-TypeScript/ (see section number 5)

We still use namespaces pretty prolifically throughout the JupyterLab codebase. As far as I know, we haven't had any conversations about discouraging their usage.

I took that online reference to mean that the Babel ts plugin has a limitation in that it can't handle namespaces, not that they are discouraged in general ts code (we aren't using Babel).

I see. One more question - do they provide any advantages over plain ES6 modules ?

ian-r-rose · 2018-12-04T18:48:05Z

packages/console/src/widget.ts

+      mimeData: new MimeData(),
+      dragImage,
+      proposedAction: 'copy',
+      supportedActions: 'copy-move',


I don't think it ultimately makes a huge difference, but in this instance copy seems like a better choice than copy-move, since there is no moving cells within the console.

I think this was copy-paste error left from the notebook package. Will take care of it

ian-r-rose · 2018-12-04T18:48:26Z

packages/console/src/widget.ts

+
+    this._drag.mimeData.setData(JUPYTER_CELL_MIME, selected);
+    const textContent = cellModel.value.text;
+    this._drag.mimeData.setData('text/plain', textContent);


Great to see this 😄

ian-r-rose · 2018-12-04T19:00:19Z

packages/cells/src/cellDragUtils.ts

+  return cellIndex;
+}
+
+export type ICellTargetArea = 'input' | 'prompt' | 'cell' | 'notebook';


This smells off to me. I think that, in general, the @jupyterlab/cells package shouldn't have to know about the context in which a cell is embedded. In this case, we are leaking the idea of a 'notebook' into this package.

I don't think that the detectTargetArea code is so long that we can't have specific versions for the notebook and the console, especially since they have different behaviors. Then we wouldn't need to export a pretty specialized public interface.

Alternatively, we could remove the 'notebook' option and add an 'unknown' option, and the notebook drag handler could just assume it's in the notebook if it is 'unknown'. What do you think @Madhu94?

Thanks! Makes sense

ian-r-rose · 2018-12-04T19:02:18Z

packages/cells/src/cellDragUtils.ts

+function findCell(
+  node: HTMLElement,
+  cells: IterableOrArrayLike<Cell>,
+  isCellNode: (node: HTMLElement) => boolean


This is a nice function reuse. I wonder if we should provide a default isCellNode that just checks for the jp-Cell class so that this function works well in the absence of a more specific filtering function. Not a huge deal either way, though.

Madhu94 · 2018-12-15T15:01:42Z

@ian-r-rose I've taken care of the review comments. Let me know if there is something else that can be done.

Madhu94 · 2019-01-05T08:59:42Z

Hi, any updates here?

ian-r-rose

So sorry for the extremely slow review cycle @Madhu94, I got super behind with the US holiday season. I have a few very minor comments, and then I think we should merge! Thanks for being persistent!

ian-r-rose · 2019-01-07T19:54:56Z

packages/cells/src/celldragutils.ts

+import { ICodeCellModel } from './model';
+import { Cell } from './widget';
+import { h, VirtualDOM } from '@phosphor/virtualdom';
+import { nbformat } from '../../coreutils/lib';


This should be imported from @jupyterlab/coreutils.

ian-r-rose · 2019-01-07T19:56:36Z

packages/cells/src/celldragutils.ts

+  /**
+   * Find the cell index containing the target html element.
+   * This function traces up the DOM hierarchy to find the root cell
+   * node. Then find the corresponding child and select it.


Can we document that it returns -1 if the cell is not found?

packages/console/src/widget.ts

ian-r-rose · 2019-01-07T20:05:12Z

packages/console/src/widget.ts

+   * Start a drag event
+   */
+  private _startDrag(index: number, clientX: number, clientY: number) {
+    const cellModel = this._focussedCell.model as ICodeCellModel;


Are we certain that this._focussedCell can never be null here? Perhaps we should double check.

Just noticed that "focused" should have one s. Thanks!

Hah, I almost wrote the same thing, but then I looked it up, and apparently two esses is a valid alternate.

Oh, wow, TIL about this! According to https://en.wiktionary.org/wiki/Talk:focus (which cites OED) and https://english.stackexchange.com/questions/4791/focussed-or-focused-rules-for-doubling-the-last-consonant-when-adding-ed, "focused" is preferred these days, at least in US and British English.

I think it wouldn't be null if I handle the case you noticed above - that cellIndex won't be -1. What do you think?

I think that's right as long as _startDrag is only called from that code path (which,to be sure, is the case right now). I'm just trying to think defensively, but it's not crucial here.

ian-r-rose · 2019-01-12T19:16:12Z

Thanks @Madhu94 (and thanks for being persistent)!

Madhu94 · 2019-01-15T20:48:49Z

Thanks for the help @ian-r-rose ! I'll be more prompt on the review comments next time, keeping a PR alive for weeks is hard.

ian-r-rose · 2019-01-15T20:49:46Z

No, the slow review cycle is entirely my fault, thanks again!

Madhu94 mentioned this pull request Nov 4, 2018

Drag cells from console to notebook #4847

Closed

Madhu94 force-pushed the console-drag-cells branch from 1123548 to db6d772 Compare November 4, 2018 14:26

ian-r-rose added enhancement pkg:console labels Nov 12, 2018

ian-r-rose added this to the 1.0 milestone Nov 12, 2018

Madhu94 force-pushed the console-drag-cells branch from db6d772 to 27ad15c Compare November 21, 2018 21:52

ian-r-rose self-assigned this Nov 29, 2018

ian-r-rose reviewed Dec 4, 2018

View reviewed changes

Madhu94 force-pushed the console-drag-cells branch 3 times, most recently from 1a825ce to feac647 Compare December 15, 2018 15:00

Madhu94 changed the title ~~[WIP] Drag drop console cells into notebook~~ Drag drop console cells into notebook Dec 22, 2018

Madhu94 force-pushed the console-drag-cells branch from feac647 to dc740fd Compare December 22, 2018 19:34

Madhu94 force-pushed the console-drag-cells branch from dc740fd to 02608a6 Compare January 5, 2019 07:49

ian-r-rose reviewed Jan 7, 2019

View reviewed changes

Madhu94 added 3 commits January 12, 2019 11:45

Allow cells from console to be dragged into notebook

3d38d4b

Fix lint issues

e778602

Addressing review comments

a6c2504

Madhu94 force-pushed the console-drag-cells branch from d6cfc62 to a6c2504 Compare January 12, 2019 07:24

ian-r-rose approved these changes Jan 12, 2019

View reviewed changes

ian-r-rose merged commit 6bd774a into jupyterlab:master Jan 12, 2019

lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 8, 2019

lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drag drop console cells into notebook #5585

Drag drop console cells into notebook #5585

Madhu94 commented Nov 4, 2018

ian-r-rose commented Nov 12, 2018

Madhu94 commented Nov 21, 2018

ian-r-rose commented Nov 30, 2018

Madhu94 commented Dec 1, 2018

Madhu94 commented Dec 1, 2018

ian-r-rose commented Dec 4, 2018

ian-r-rose left a comment

ian-r-rose Dec 4, 2018

Madhu94 Dec 8, 2018

ian-r-rose Dec 10, 2018

jasongrout Dec 10, 2018

Madhu94 Dec 13, 2018

ian-r-rose Dec 4, 2018

Madhu94 Dec 8, 2018

ian-r-rose Dec 4, 2018

ian-r-rose Dec 4, 2018

Madhu94 Dec 8, 2018

ian-r-rose Dec 4, 2018

Madhu94 commented Dec 15, 2018

Madhu94 commented Jan 5, 2019

ian-r-rose left a comment

ian-r-rose Jan 7, 2019

ian-r-rose Jan 7, 2019

ian-r-rose Jan 7, 2019

jasongrout Jan 7, 2019

ian-r-rose Jan 7, 2019

jasongrout Jan 8, 2019

Madhu94 Jan 12, 2019

ian-r-rose Jan 12, 2019

ian-r-rose commented Jan 12, 2019

Madhu94 commented Jan 15, 2019

ian-r-rose commented Jan 15, 2019

		@@ -5,6 +5,7 @@

		import '../style/index.css';

		export {default as CellDragUtils, ICellTargetArea} from './cellDragUtils';

Drag drop console cells into notebook #5585

Drag drop console cells into notebook #5585

Conversation

Madhu94 commented Nov 4, 2018

ian-r-rose commented Nov 12, 2018

Madhu94 commented Nov 21, 2018

ian-r-rose commented Nov 30, 2018

Madhu94 commented Dec 1, 2018

Madhu94 commented Dec 1, 2018

ian-r-rose commented Dec 4, 2018

ian-r-rose 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

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

Madhu94 commented Dec 15, 2018

Madhu94 commented Jan 5, 2019

ian-r-rose 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-r-rose commented Jan 12, 2019

Madhu94 commented Jan 15, 2019

ian-r-rose commented Jan 15, 2019