SwanFileBrowser extension for jupyterlab #199
Conversation
| @@ -0,0 +1,34 @@ | |||
| import { URLExt } from '@jupyterlab/coreutils'; | |||
|
|
|||
There was a problem hiding this comment.
What's this file for? Is it a copy from upstream? Any modification you did?
There was a problem hiding this comment.
This file has the function that allows requesting our backend extensions.
it is not a copy of an upstream function,
I took it from the extension example like this https://github.com/jupyterlab-contrib/search-replace/blob/master/src/handler.ts
I don't remember which one :C
There was a problem hiding this comment.
Do we really need this file? Isn't there an equivalent in the upstream that we can import?
omazapa
force-pushed
the
swanfilebrowser
branch
2 times, most recently
from
c7503fb to
249cbe7
Compare
|
Thanks a lot @etejedor for your review :D |
| * This file is the entry point for the extension where is defined the commands such as | ||
| * 'filebrowser:copy', 'filebrowser:cut', 'filebrowser:open' etc.. | ||
| * Those commands are reimplemented using our class SwanFileBrowser instead the default Jupyter FileBrowser. | ||
| * This extension replaces the default FileBrowser, then we need to redefined the commands provided by the default FileBrowser |
There was a problem hiding this comment.
to redefined -> to redefine
omazapa
force-pushed
the
swanfilebrowser
branch
from
249cbe7 to
204c010
Compare
There was a problem hiding this comment.
Some questions and comments :)
Important: comment the code with the exact information from upstream to allow us to compare upstream with your code.
And the objective is to copy the code (when we need to) and try to keep it as similar as possible with upstream.
| ft, | ||
| this.translator, | ||
| this._hiddenColumns, | ||
| swan_item.is_project === true && swan_item.type === 'directory' |
There was a problem hiding this comment.
Maybe instead of passing this boolean, since you already pass item, I would keep all methods the same and just put the project logic inside updateItemNode only.
| const modified = DOMUtils.findElement(dragImage, ITEM_MODIFIED_CLASS); | ||
| console.log('-----drag'); | ||
| console.log(node); | ||
| const isProject = node.getAttribute('is_project'); |
There was a problem hiding this comment.
Where is this attribute added?
| const isProject = node.getAttribute('is_project'); | ||
| let icon = null; | ||
| if (isProject === 'true') { | ||
| icon = DOMUtils.findElement(dragImage, ITEM_PROJECT_ICON_CLASS); |
There was a problem hiding this comment.
I also don't see where ITEM_PROJECT_ICON_CLASS is being added? In updateItemNode I see you adding ITEM_ICON_CLASS to a Project as well.
I would suggest we keep ITEM_ICON_CLASS and again modify just the absolutely necessary.
| @@ -0,0 +1,2391 @@ | |||
| /** | |||
| * File taken from https://github.com/jupyterlab/jupyterlab/blob/master/packages/filebrowser/src/listing.ts | |||
| * for Jupyterlab version 3.0.x. | |||
There was a problem hiding this comment.
Please add the hash instead of the branch tag. I wanted to compare with upstream and I'm seeing other changes in the file, if I had a commit hash I would be able to see exactly the same file.
| * -> onUpdateRequest | ||
| * -> updateItemNode | ||
| */ | ||
| export class SwanDirListing extends Widget { |
There was a problem hiding this comment.
Why do we need to create a SwanDirListing instead of just using the one that comes in upstream (DirListing)?
I would suggest we change just the absolutely minimum.
We only copy this file so that we can show the project folder icon, right? I got the impression we could just cast the model to ISwanModel inside updateItemNode (to do the if that changes the icon), and leave everything else exactly as is.
| @@ -0,0 +1,34 @@ | |||
| import { URLExt } from '@jupyterlab/coreutils'; | |||
|
|
|||
There was a problem hiding this comment.
Do we really need this file? Isn't there an equivalent in the upstream that we can import?
| return rvalue; | ||
| }); | ||
| } catch (reason) { | ||
| console.error(`Error on GET 'api/contents'+ ${cwd}.\n${reason}`); |
| console.log(message); | ||
| }); | ||
| } | ||
| await this.kernelSpecSetPathRequest(this.path); |
There was a problem hiding this comment.
Do we need this? I'm not sure we do... I though wether changing the spec manager from another tab would be possible, but Lab doesn't allow you to have more than one window open, right?
If we do need it, we should do it bellow when we also cd to '.'.
|
Closing as superseded by #209 |
Hi Guys,
This is the SwanFileBrowser extension.
This extension replaces the default FileBrowser and allows you to control directories that are projects.