-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Adds a Move button to the notebooklist interface next to Rename. #941
Conversation
In this Pull Request the button only supports Moving 1 selected item at a time, but I've left a TODO in the code and also opened Issue #942 to address moving multiple. Should I reference the issue number from the TODO? I'm still fairly new to open source codebases. :) Thanks! |
return; | ||
} | ||
|
||
var that = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make more sense to move this variable declaration to the top of the function and use that
instead of this
throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks. (Extremely new to Javascript; why do we even create the that
variable? I was just following the convention of the surrounding functions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, @NHDaly! I'm gonna give explaining it a shot and let me know if I am unclear. It all boils down to scoping. In JavaScript, the this
keyword is used to refer to the object that the function you are currently in is tied to. In this case, this
references NotebookList
. You can confirm this by adding a console.log(this)
statement to the beginning of the move_selected
function. Now, let's say that inside your move_selected
function, you create a new object with functions as you do with the dialog in this case. Your this
now references a completely different object within those functions. You can test this out by adding a console.log(this)
right before line 741. But what if in this new context you want to reference the NotebookList
as you do on line 741? Well, you store the value of this
in a variable called that
when this
references the object you later want to reference and then use that
in functions where this
does not reference that object.
So in reality, you don't particularly need it for the first few lines at the top since this
references NotebookList
in those cases but I like to declare at the top and use it consistently so that it is obvious that throughout the code anywhere that
is used we are referencing a NotebookList
object.
Let me know if any of that needs clarification! this
is definitely a tricky topic in JavaScript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Safia, nope that makes tons of sense now. Gotcha! Clever! Unnamed functions ... pretty crazy.
Thanks for taking the time to explain that to me! :)
Good stuff, @NHDaly! I posted some notes inline. |
}); | ||
} | ||
}, | ||
Cancel : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're looking to change our button orders so that the "Cancel" is always on the left and the action is on the right. So these should be switched to be compatible with future changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. That makes sense; thanks!
Cool, thanks for looking at this! :) I've addressed your suggestions in ce24663! :) Cheers! |
}); | ||
input.focus(); | ||
// Highlight the current path in the input box. | ||
input.select(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open Question: This is how the Rename box works, which makes sense. But should the Move button start with a pre-selected path? I feel like more often you're going to be modifying the path rather than overwriting it with an entirely new path, so maybe this should just move the cursor to the end instead of selecting the input.
Thoughts? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm almost tempted to say that the Move functionality should prompt the user to select a directory to move a file into with a dropdown. I say this for two reasons.
- To make the creating a directory and moving something into a directory two distinctly different tasks.
- To conform with more tradition UI where the user doesn't enter a path to move files to manually.
I'm not sure how difficult creating a dropdown would be, but happy to help you work through it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The move UI in the GitHub webapp is really cool.
On Wed, Jan 20, 2016 at 7:55 AM, Safia Abdalla [email protected]
wrote:
In notebook/static/tree/js/notebooklist.js
#941 (comment):
console.warn('Error durring moving :', e);
});
}
}
},
open : function () {
// Upon ENTER, click the OK button.
input.keydown(function (event) {
if (event.which === keyboard.keycodes.enter) {
d.find('.btn-primary').first().click();
return false;
}
});
input.focus();
// Highlight the current path in the input box.
input.select();
So I'm almost tempted to say that the Move functionality should prompt the
user to select a directory to move a file into with a dropdown. I say this
for two reasons.
- To make the creating a directory and moving something into a
directory two distinctly different tasks.- To conform with more tradition UI where the user doesn't enter a
path to move files to manually.I'm not sure how difficult creating a dropdown would be, but happy to help
you work through it.—
Reply to this email directly or view it on GitHub
https://github.com/jupyter/notebook/pull/941/files#r50272758.
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Now that I think about it @ellisonbg's recommendation is a good one and you won't have to steer too far from the code that you have currently written to make it happen.
Here is the help page for GitHub's move feature. So the way I imagine the user experience for this working is as follows.
- User selects a checkbox on a file in the file tree.
- User clicks the move button in the toolbar above.
- Filename in the file tree gets converted into a file path relative to the directory the user is in.
- User writes within the textbox the new path.
- User hits enter and the file is moved to a new location if there is no error.
Thoughts on this, @NHDaly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of what is cool about the Github approach is that as you type paths
with / it detects that and moves those into the path part. Really slick.
On Wed, Jan 20, 2016 at 8:58 AM, Safia Abdalla [email protected]
wrote:
In notebook/static/tree/js/notebooklist.js
#941 (comment):
console.warn('Error durring moving :', e);
});
}
}
},
open : function () {
// Upon ENTER, click the OK button.
input.keydown(function (event) {
if (event.which === keyboard.keycodes.enter) {
d.find('.btn-primary').first().click();
return false;
}
});
input.focus();
// Highlight the current path in the input box.
input.select();
Hm. Now that I think about it @ellisonbg https://github.com/ellisonbg's
recommendation is a good one and you won't have to steer too far from the
code that you have currently written to make it happen.Here https://help.github.com/articles/moving-a-file-to-a-new-location/
is the help page for GitHub's move feature. So the way I imagine the user
experience for this working is as follows.
- User selects a checkbox on a file in the file tree.
- User clicks the move button in the toolbar above.
- Filename in the file tree gets converted into a file path relative
to the directory the user is in.- User writes within the textbox the new path.
- User hits enter and the file is moved to a new location if there is
no error.Thoughts on this, @NHDaly https://github.com/NHDaly?
—
Reply to this email directly or view it on GitHub
https://github.com/jupyter/notebook/pull/941/files#r50282784.
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia: I believe the flow you have described is exactly how the Rename
button already works. You can use Rename
to move a file to a different directory by changing the relative path. E.g:
To move a file to a parent directory:
- Select checkbox on file.
- Press Rename. The textbox is pre-populated with just the filename + extension (e.g. "my_notebook.pynb")
- Prepend
../
to the name (e.g. changing it to "../my_notebook.pynb") - Enter.
That's all done with relative paths. Is that what you were describing? However, I don't think this is intuitive for a user not familiar with Unix, who wouldn't think to rename a file to "../" to move it up a directory. That's why I wanted to add a Move button.
@ellisonbg + @captainsafia: Yeah, wow, that is a slick UI. I love it! That said, I think it would be good to separate that fancy UI into a separate PR? It works almost the same as this code, but builds a fancier UI on top of it. If we're happy with the semantics in this PR, I'd prefer starting a new one for the fancier interface. Does that sound okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellisonbg + @captainsafia: Also, I wonder about how well the github style interface would support moving multiple files? I have opened #942 to support moving multiple files from the Move dialogue.
I guess maybe you could do the same thing as the Github UI, but leave off the file name?
With this button, you can now move files or directories to any path in the notebook, including creating new directories (like 'mv -p'). To move a file, you specify the new destination directory's path. This achieves the same effect as renaming a file with a new relative path, but is more clear to users unfamiliar with unix commands -- especially with regard to moving to a parent directory. Can only move one file at a time currently.
Part of addressing Issue #941.
…iod. The Move button expects a file path, which shouldn't include a file extension suffix.
Friendly ping? :) |
Adds a Move button to the notebooklist interface next to Rename.
@NHDaly thanks! |
No problem! (Yay!! My first merged-in GitHub PR! haha 💃) |
Congrats! Looking forward to more great PRs from you, @NHDaly! |
hahaha thanks. Awesome gif. |
@NHDaly Nice job! Welcome to Open Source and Jupyter 🏄 @captainsafia sends great gifs. |
sorry for dumb nube question but how do i actually use your cool move button to move files in Jupyter? |
With this button, you can now move files or directories to any path in
the notebook, including creating new directories (like 'mv -p'). To move
a file, you specify the new destination directory's path.
This achieves the same effect as renaming a file with a new relative
path, but is more clear to users unfamiliar with unix commands --
especially with regard to moving to a parent directory.
Can only move one file at a time currently.
Closes #471.