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

Add a 'Copy Path' to filebrowser context menu. #4582

Merged
merged 1 commit into from May 17, 2018

Conversation

@aisipos
Copy link
Contributor

@aisipos aisipos commented May 16, 2018

Fixes #3845

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 16, 2018

@aisipos thanks for this contribution! Could you post a screenshot of the context menu for visual design review?

@aisipos
Copy link
Contributor Author

@aisipos aisipos commented May 16, 2018

Hi @ellisonbg , thanks! Here's what this PR looks like on my machine:

image

@ivanov
Copy link
Member

@ivanov ivanov commented May 16, 2018

I had to kick travis to restart the build, hopefully it was a transient error.

@aisipos aisipos force-pushed the filebrowser-copy-path branch 2 times, most recently from 146fcf9 to 50e18bc May 16, 2018
@aisipos
Copy link
Contributor Author

@aisipos aisipos commented May 16, 2018

Noting, this PR is being developed at the pycon 2018 sprints with @ivanov.

Copy link
Member

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

Thanks @aisipos, this is looking great! I just have one minor comment, and then I think we should be good to go.

@@ -405,6 +409,17 @@ function addCommands(app: JupyterLab, tracker: InstanceTracker<FileBrowser>, bro
label: 'Copy Shareable Link'
});

commands.addCommand(CommandIDs.copyPath, {
execute: () => {
const path = browser.selectedItems().next().path;
Copy link
Member

@ian-r-rose ian-r-rose May 16, 2018

Choose a reason for hiding this comment

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

Strictly speaking, I think it is possible for this command to be executed when there are not any selected items, so the first item grabbed from the iterator can be undefined. A slightly more defensive way of doing this would be to write

const item = browser.selectedItems().next();
if (!item) {
  return;
}
Clipboard.copyToSystem(item.path);

Copy link
Contributor Author

@aisipos aisipos May 17, 2018

Choose a reason for hiding this comment

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

@ian-r-rose Good point! I made the change you suggested and pushed.

@aisipos aisipos force-pushed the filebrowser-copy-path branch from 50e18bc to 23694f1 May 17, 2018
Copy link
Member

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

Thanks @aisipos !

@ian-r-rose ian-r-rose merged commit c4e6eb6 into jupyterlab:master May 17, 2018
2 checks passed
@aisipos
Copy link
Contributor Author

@aisipos aisipos commented May 17, 2018

@ian-r-rose Thanks for merging c4e6eb6 . I'll note though in that commit the --author flag wasn't set. Is that something normally set on PRs for jupyterlab?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 17, 2018

I'm not sure I understand. I see your commit in the log here, attributed to you: f6d7b5d

@aisipos
Copy link
Contributor Author

@aisipos aisipos commented May 18, 2018

@ian-r-rose My apologies. I wasn't showing up in the contributors page:
https://github.com/jupyterlab/jupyterlab/graphs/contributors?from=2018-05-01&to=2018-05-18&type=a

But this is because only the top 100 contributors by commits are ever shown, and this repo has more than 100 contributors ( 🎉 ) :
https://help.github.com/enterprise/2.8/user/articles/i-don-t-see-myself-in-the-repository-contributors-graph/

But it does see my commits if you look specifically:
https://github.com/jupyterlab/jupyterlab/commits?author=aisipos

Thanks again!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 18, 2018

Thanks for the contribution!

@jasongrout jasongrout added this to the Beta 3 milestone Jun 12, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 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.

5 participants