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 Open In New Browser Tab option to the file browser #4315

Merged
merged 5 commits into from Apr 17, 2018

Conversation

@jtpio
Copy link
Member

@jtpio jtpio commented Apr 3, 2018

Add the "Open in New Tab" option to the filebrowser right-click menu to cover the use case discussed in #4137.

It is not only limited to html files so images can also be opened in a new browser tab.

Flow

new-tab

* Add new command `filebrowser:open-tab`
* Use `jp-AddIcon` for now as a placeholder
return commands.execute('docmanager:open-tab', { path: item.path });
})));
},
iconClass: 'jp-MaterialIcon jp-AddIcon',
Copy link
Member Author

@jtpio jtpio Apr 3, 2018

Choose a reason for hiding this comment

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

jp-AddIcon is currently used as a placeholder, so we might need a new icon (or remove it) if this one is not appropriate.

Copy link
Member

@ian-r-rose ian-r-rose Apr 9, 2018

Choose a reason for hiding this comment

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

Honestly, the jp-AddIcon looks okay to me. A plus symbol for new browser tab seems pretty common when I looked around. The only issue is that we have talked about adding a few other items to the filebrowser context menu like "New File" and "New Directory", and those may also want a similar icon. Did you have any other ideas about what might work?

Copy link
Member Author

@jtpio jtpio Apr 9, 2018

Choose a reason for hiding this comment

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

True, Firefox also uses a plus symbol to open a new tab (although it's blank). The alternatives would be:

For the New Directory, there is already the "New Folder" icon that could be reused in the filebrowser context menu:
image
But there doesn't seem to be any existing icon for "New File". I think something similar to the "Copy" icon but with a + on the top would work fine.

jtpio added 2 commits Apr 3, 2018
@jtpio jtpio changed the title Add Open In New Tab option to the file browser Add Open In New Browser Tab option to the file browser Apr 7, 2018
Copy link
Member

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

I think this is a really nice piece of work, and a good addition!

return commands.execute('docmanager:open-tab', { path: item.path });
})));
},
iconClass: 'jp-MaterialIcon jp-AddIcon',
Copy link
Member

@ian-r-rose ian-r-rose Apr 9, 2018

Choose a reason for hiding this comment

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

Honestly, the jp-AddIcon looks okay to me. A plus symbol for new browser tab seems pretty common when I looked around. The only issue is that we have talked about adding a few other items to the filebrowser context menu like "New File" and "New Directory", and those may also want a similar icon. Did you have any other ideas about what might work?

const path = typeof args['path'] === 'undefined' ? ''
: args['path'] as string;
return docManager.services.contents.getDownloadUrl(path).then(url => {
window.open(url, '_blank');
Copy link
Member

@ian-r-rose ian-r-rose Apr 9, 2018

Choose a reason for hiding this comment

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

Out of curiosity, does this also work for PDFs?

Copy link
Member Author

@jtpio jtpio Apr 9, 2018

Choose a reason for hiding this comment

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

Yes.

new-tab-pdf

commands.addCommand(CommandIDs.openTab, {
execute: args => {
const path = typeof args['path'] === 'undefined' ? ''
: args['path'] as string;
Copy link
Member

@ian-r-rose ian-r-rose Apr 9, 2018

Choose a reason for hiding this comment

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

I think it is probably better to do nothing here if path is not defined, or if it is the empty string.

Copy link
Member Author

@jtpio jtpio Apr 10, 2018

Choose a reason for hiding this comment

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

Good point, I added a check to handle this case.

@ian-r-rose
Copy link
Member

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

This looks great, thanks @jtpio!

@ian-r-rose ian-r-rose merged commit 93773c6 into jupyterlab:master Apr 17, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 17, 2018

In a follow-up PR, can we rename the api commands to include 'browser'? openBrowserTab, filebrowser:open-browser-tab, etc.? I'm afraid it's going to get confusing to developers too since we have the concept of tabs in jlab.

@jasongrout jasongrout added this to the Beta 3 milestone Apr 17, 2018
@jtpio jtpio deleted the open-in-new-tab branch Apr 17, 2018
@jtpio
Copy link
Member Author

@jtpio jtpio commented Apr 17, 2018

I agree it will be more explicit to include 'browser' in the command names 👍

Will do the renaming in a follow-up PR!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 17, 2018

Thanks!

@HankBu
Copy link

@HankBu HankBu commented May 20, 2019

Excuse me, but if use 'Open in New Browser Tab', will it cause Cross Site Scripting (XSS)?How can I hide the button? Thanks

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

None yet

4 participants