Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drag drop console cells into notebook #5585

Merged
merged 3 commits into from Jan 12, 2019

Conversation

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Nov 4, 2018

Fixes #4847

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Nov 12, 2018

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
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Nov 21, 2018

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 ian-r-rose self-assigned this Nov 29, 2018
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Nov 30, 2018

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
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Dec 1, 2018

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
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Dec 1, 2018

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

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 4, 2018

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

Copy link
Member

@ian-r-rose ian-r-rose left a comment

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.

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

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

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

@ian-r-rose ian-r-rose Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@Madhu94 Madhu94 Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@ian-r-rose ian-r-rose Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jasongrout jasongrout Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

@Madhu94 Madhu94 Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mimeData: new MimeData(),
dragImage,
proposedAction: 'copy',
supportedActions: 'copy-move',
Copy link
Member

@ian-r-rose ian-r-rose Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Madhu94 Madhu94 Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

@ian-r-rose ian-r-rose Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this 😄

return cellIndex;
}

export type ICellTargetArea = 'input' | 'prompt' | 'cell' | 'notebook';
Copy link
Member

@ian-r-rose ian-r-rose Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@Madhu94 Madhu94 Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Makes sense

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

@ian-r-rose ian-r-rose Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Madhu94 force-pushed the console-drag-cells branch 3 times, most recently from 1a825ce to feac647 Dec 15, 2018
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Dec 15, 2018

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

@Madhu94 Madhu94 changed the title [WIP] Drag drop console cells into notebook Drag drop console cells into notebook Dec 22, 2018
@Madhu94 Madhu94 force-pushed the console-drag-cells branch from feac647 to dc740fd Dec 22, 2018
@Madhu94 Madhu94 force-pushed the console-drag-cells branch from dc740fd to 02608a6 Jan 5, 2019
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jan 5, 2019

Hi, any updates here?

Copy link
Member

@ian-r-rose ian-r-rose left a comment

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!

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

@ian-r-rose ian-r-rose Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be imported from @jupyterlab/coreutils.

/**
* 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.
Copy link
Member

@ian-r-rose ian-r-rose Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/console/src/widget.ts Show resolved Hide resolved
* Start a drag event
*/
private _startDrag(index: number, clientX: number, clientY: number) {
const cellModel = this._focussedCell.model as ICodeCellModel;
Copy link
Member

@ian-r-rose ian-r-rose Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jasongrout jasongrout Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ian-r-rose ian-r-rose Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jasongrout jasongrout Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Madhu94 Madhu94 Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@ian-r-rose ian-r-rose Jan 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Madhu94 Madhu94 force-pushed the console-drag-cells branch from d6cfc62 to a6c2504 Jan 12, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 12, 2019

Thanks @Madhu94 (and thanks for being persistent)!

@ian-r-rose ian-r-rose merged commit 6bd774a into jupyterlab:master Jan 12, 2019
2 of 3 checks passed
agoose77
Copy link
Contributor

agoose77 commented on 3d38d4b Jan 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to take the euclidean distance here? e.g.

    let dx = nextX - prevX;
    let dy = nextY - prevY;
    return Math.sqrt(dx*dx + dy*dy) >= DRAG_THRESHOLD;

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jan 15, 2019

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
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 15, 2019

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

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants