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

[WIP] Provide a signal for opening items from a DirListing #6270

Merged
merged 2 commits into from May 23, 2019
Merged

[WIP] Provide a signal for opening items from a DirListing #6270

merged 2 commits into from May 23, 2019

Conversation

@yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Apr 29, 2019

Possible way to fix #6269

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Apr 29, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

Copy link
Member

@blink1073 blink1073 left a comment

LGTM, thanks! Just one nit.

@@ -897,6 +913,17 @@ export class DirListing extends Widget {
this._startDrag(data.index, event.clientX, event.clientY);
}

private emitOpenEvent(item: Contents.IModel): void {
Copy link
Member

@blink1073 blink1073 Apr 29, 2019

Private methods should have a leading underscore. Also, this does more than emit a signal. How about:

/**
 * Open a file item and emit a signal.
 */
private _openItem(item: Contents.IModel): void {

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 29, 2019

Does this prevent the item from opening locally? I thought you wanted to take over the opening, not just have a signal.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 29, 2019

(from the formatting changes, it seems that you have an older version of prettier installed. I would check in any changes, git clean -dfx your repo directory, and reinstall to get a fresh set of dependencies including an updated prettier)

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 23, 2019

Made a few tweaks, this is good to go, thanks @yuvipanda!

@blink1073 blink1073 merged commit df62287 into jupyterlab:master May 23, 2019
9 checks passed
@yuvipanda
Copy link
Contributor Author

@yuvipanda yuvipanda commented May 24, 2019

Thanks, @blink1073!

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 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.

3 participants