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 directory navigation to dashboard #5001

Merged
merged 25 commits into from Feb 6, 2014
Merged

Conversation

ellisonbg
Copy link
Member

OK, this is what happens when the Super Bowl turns out like this...

I talked to @fperez today and we both feel that it is important to have some sort of minimal directory navigation in the dashboard for 2.0. This is an attempt to do that with a minimal amount of technical debt. Here is the rough outline:

  • I have added directory items to the FileNotebookManagers list of notebooks. This is localized into two methods that can be easily removed once we have the full contents web service.
  • The order in the dashboard is this:
    • ..
    • 'index.ipynb` if it exists (this is case independent)
    • Then all directories sorted.
    • Then all notebook sorted.
  • I have added icons for notebooks and directories and added the .ipynb to notebook items, like we do for nbviewer.

Here is a screenshot:

screen shot 2014-02-02 at 6 21 42 pm

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Hey @ellisonbg, I see Travis is reporting a build failure, you might want to have a look.

I'll play with the code now. Thanks!! I think this will add a ton of usability to 2.0 out the door.

@fperez
Copy link
Member

fperez commented Feb 3, 2014

From first testing: this is AWESOME. Seriously, thanks!!

Minor suggestion: perhaps tightening vertical spacing somewhat? Even on a large monitor, I have to scroll a fair bit up and down in large directories, and on laptops it's bound to be worse. Shaving a few pixels in each row will help a lot. I'd use as a baseline the 'compact' choice for display density in gmail (we could make that configurable like gmail later on, but since that's not in the cards for now, I'd err on the side of efficiency and pack more info on the screen).

I'll keep playing with it, but I'm already loving it :)

@ellisonbg
Copy link
Member Author

Each item in the notebook list has a top and bottom padding of 5px. This is
already down from 8px in master. Down to maybe 3px?

On Sun, Feb 2, 2014 at 7:13 PM, Fernando Perez notifications@github.comwrote:

From first testing: this is AWESOME. Seriously, thanks!!

Minor suggestion: perhaps tightening vertical spacing somewhat? Even on a
large monitor, I have to scroll a fair bit up and down in large
directories, and on laptops it's bound to be worse. Shaving a few pixels in
each row will help a lot. I'd use as a baseline the 'compact' choice for
display density in gmail (we could make that configurable like gmail later
on, but since that's not in the cards for now, I'd err on the side of
efficiency and pack more info on the screen).

I'll keep playing with it, but I'm already loving it :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33922255
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Yes, I'd play with 3px, see how it looks. Perhaps instead of px it should be in fractions of a letter height? I'm thinking of retina displays, where 2 or 3px is nearly nothing... I think @minrk has a retina screen, he can let us know how it looks there...

@ellisonbg
Copy link
Member Author

But I think that screen px are not the same as css px - otherwise the
dashboard on a retina would already look all wacky.

On Sun, Feb 2, 2014 at 7:24 PM, Fernando Perez notifications@github.comwrote:

Yes, I'd play with 3px, see how it looks. Perhaps instead of px it should
be in fractions of a letter height? I'm thinking of retina displays, where
2 or 3px is nearly nothing... I think @minrk https://github.com/minrkhas a retina screen, he can let us know how it looks there...

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33922494
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member Author

Also, all of our styling on the dashboard is already in px.

On Sun, Feb 2, 2014 at 7:33 PM, Brian Granger ellisonbg@gmail.com wrote:

But I think that screen px are not the same as css px - otherwise the
dashboard on a retina would already look all wacky.

On Sun, Feb 2, 2014 at 7:24 PM, Fernando Perez notifications@github.comwrote:

Yes, I'd play with 3px, see how it looks. Perhaps instead of px it should
be in fractions of a letter height? I'm thinking of retina displays, where
2 or 3px is nearly nothing... I think @minrk https://github.com/minrkhas a retina screen, he can let us know how it looks there...

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33922494
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Ah, OK, I didn't realize that (and I'm glad that's the case). Then I'd play with very tight numbers (3px or even 2) and see how it feels on your box with directories that have lots of stuff in them...

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Here's a screenshot of my gmail inbox next to our examples dir:

image

I think something roughly like gmail would be ideal...

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Oops, updated comment with correct screenshot above.

@ellisonbg
Copy link
Member Author

@fperez I just pushed some commits that give proper breadcrumbs and take
the spacing down to 4px. This means each notebook list item has a total
height of 32px. Compare this to 31px for the GitHub repo list item height.
The reason it is hard to go lower is that we have to fit those pesky
buttons on there as well. I can go down to 3px without messing anything up
though. See what you think.

On Sun, Feb 2, 2014 at 7:42 PM, Fernando Perez notifications@github.comwrote:

Ah, OK, I didn't realize that (and I'm glad that's the case). Then I'd
play with very tight numbers (3px or even 2) and see how it feels on your
box with directories that have lots of stuff in them...

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33922992
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member Author

OK, I pushed a commit to take it to 3px. Looks like it works OK. Also added
red background to the Shutdown button to people can distinguish if from
delete. Not sure if this is the right thing - as delete it more dangerous,
but it would be horribly annoying to have red all the way down the page...

On Sun, Feb 2, 2014 at 7:47 PM, Fernando Perez notifications@github.comwrote:

Oops, updated comment with correct screenshot above.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33923105
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member Author

For the curious here is the new look:

screen shot 2014-02-02 at 7 55 55 pm

Wow, how did we live with the dashboard like it was for so long...

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Already looks better! I would indeed try to go even tighter, if possible. Note that the spacing below the delete buttons seems a bit larger than above (this is a screenshot taken with a magnifier):

image

Keeping those two the same would be good, perhaps 1px less than what's currently the top spacing and matching the bottom one.

Otherwise, usability feels great, I'm using it right now and it's awesome. And with stable URLs, I just started a server on a tmux session (so it survives even accidental terminal closures), and can use my browser bookmarks for persistent navigation if I want.

Absolutely fantastic improvement. We knew we wanted this, but I'm amazed at how much it feels like this was just something we can't live without :)

@fperez
Copy link
Member

fperez commented Feb 3, 2014

On Sun, Feb 2, 2014 at 7:56 PM, Brian E. Granger
notifications@github.comwrote:

Wow, how did we live with the dashboard like it was for so long...

Could not agree more. That's precisely what I was typing as well :)

Fernando Perez (@fperez_org; http://fperez.org)
fperez.net-at-gmail: mailing lists only (I ignore this when swamped!)
fernando.perez-at-berkeley: contact me here for any direct mail

@fperez
Copy link
Member

fperez commented Feb 3, 2014

I'm wondering if we should change the Dashboard page title to reflect the path. That will make it much easier to find the tab you want once we start having a bunch of them open at different filesystem locations (I'm already seeing that in use).

@rudimk
Copy link

rudimk commented Feb 3, 2014

This is fabulous! Now, how about we go extend this further, and let people upload files - text files, images, CSVs, that sort of thing - through this interface? I tried hacking on it last night - but I was using Anaconda, and I don't think playing with the IPython source in the Anaconda distribution's a great idea. Anyway, I was trying to modify the existing notebook upload feature, to accept all kinds of extensions, and not just .ipynb. That's a security risk, yeah, so we could always include a list of commonly-used files, and let the javascript upload function check if the file being uploaded matches any of those extensions or not.

@ellisonbg
Copy link
Member Author

We have plans to do a full refactor/rewrite of the dashboard that will
include full handling (including editing and uploading) for all reasonable
file types. Part of the problem we have right now is that the dashboard
code is a hideous mess. We plan on rewriting it with our modern tool stack
(bootstrap, backbone, etc.). This will take place after the 2.0 release
later in the Spring/Summer.

This current PR is just to get the basics of directory navigation in for
the 2.0 release.

Glad you like it!

On Sun, Feb 2, 2014 at 10:10 PM, Rudi MK notifications@github.com wrote:

I guess I could work on that, once this particular pull request is merged..

Reply to this email directly or view it on GitHubhttps://github.com//pull/5001#issuecomment-33926839
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@jdfreder
Copy link
Member

jdfreder commented Feb 3, 2014

Wow! This looks good.

@rudimk
Copy link

rudimk commented Feb 3, 2014

+n. Would love to get me hands dirty.

@ellisonbg
Copy link
Member Author

OK, I have tightened up some of the other spacing on the page (the Notebooks/Cluster tab and refresh/download area). There is not fine tuning of the CSS to do, but I am getting close. Here is the latest look.

screen shot 2014-02-02 at 10 51 18 pm

@minrk
Copy link
Member

minrk commented Feb 3, 2014

Very nice! Don't forget to add tests before merging.

@fperez
Copy link
Member

fperez commented Feb 3, 2014

Great improvements, awesome. Do you know what's causing the Travis issues (I haven't had the time to look)?

@damianavila
Copy link
Member

Yeah, looks great!

dirs = []
for name in dir_names:
os_path = self.get_os_path(name, path)
if os.path.isdir(os_path) and not name.startswith('.'):
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about not including the hidden folders in the list?

Taking into account that:

  • the notebook will open other sort of files than ipynb,
  • some config files are behind hidden folders,

Just thinking loud 😉 because I am not sure to including now too...

Copy link
Member

Choose a reason for hiding this comment

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

The /files handler already prevents access to hidden files. This was a rather quick and dirty fix so that if you start the server in your home directory, you're not serving your SSH keys.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thanks for the info @takluyver... I have checked the handlers... thanks for pointing out 😉

Copy link
Member

Choose a reason for hiding this comment

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

the files handler specifically hides 'hidden' directories as well. Maybe use the same logic (move it, if it's not in a reusable location)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will consolidate this hidden dir logic.

@jdfreder
Copy link
Member

jdfreder commented Feb 3, 2014

Looks good, and works, I tried it on both Firefox and Chrome w/ Linux Mint 👍

@ellisonbg
Copy link
Member Author

OK I have added test - still waiting for Travis to finish, but it passed the new tests I wrote and the html tests that were failing before. But I have a few more minor things to finish before this gets merged. Will ping when it is ready.

@minrk
Copy link
Member

minrk commented Feb 4, 2014

But I think that screen px are not the same as css px - otherwise the dashboard on a retina would already look all wacky.

Right - one CSS px is actually one point, not one physical pixel. In 2x mode, this is a 2x2 square of physical pixels, but it can also be all kinds of other crazy things (on my screen right now, 1 CSS px is actually 1.3125 pixels²).

@minrk
Copy link
Member

minrk commented Feb 5, 2014

Maybe we could add a content_format that specifies if the content is JSON data or a string.

That's a good idea. Then there would only be one save. It could also be implicit on the 'type' key, since it will presumably only ever be notebooks that use JSON. If we ever add other special JSON formats, presumably they will also get a 'type' entry.

The only place the extension is not wanted is editing a name during upload.
Everywhere else, it's actually wanted.
@minrk
Copy link
Member

minrk commented Feb 5, 2014

I've just added some unicode and space-having paths, and these are definitely failing. I'm going to try to fix those, and send you a PR.

@ellisonbg
Copy link
Member Author

OK @minrk I think I have done everything but the notebookPath changes. I am going to let Travis run and work on other stuff while you are at lunch, but I am here if more changes need to be made to this.

In order to put up the 404 when the user tries to visit hidden dirs in the dashboard, I had to also made is_hidden a method of the base and file notebook manager. This is so the tree handler can call it to see if the 404 is needed. This method just calls the underlying general purpose is_hidden function in IPython.utils.

@fperez
Copy link
Member

fperez commented Feb 5, 2014

Thought/question on this one: navigating back to the very top from the breadcrumbs requires clicking on a single '/', which is both narrow (requiring fine motor control) and also not very obvious. I wonder if we could find a way to represent that root dir in a bit more obvious/intuitive way that's also easier t click on.

I was thinking something like 'notebooks://' or 'ipynb://' or similar, but I don't really have a good idea I like. Just thinking out loud...

@ellisonbg
Copy link
Member Author

Yes, it is not ideal by any means. Maybe "Home"?

@fperez
Copy link
Member

fperez commented Feb 5, 2014

+1 to "Home"

@Carreau
Copy link
Member

Carreau commented Feb 5, 2014

+1 to "Home"

Make it a Home Font awesome Icon, it does not need to be translated :-)

@minrk
Copy link
Member

minrk commented Feb 5, 2014

+1 to using an icon instead of a word for the root location.

"""
inside_root = absolute_path[len(absolute_root):]
if any(part.startswith('.') for part in inside_root.split(os.sep)):
if is_hidden(abs_root, abs_path):
raise web.HTTPError(403)
Copy link
Member

Choose a reason for hiding this comment

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

I think you were going to make this 404

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good catch.

@ellisonbg
Copy link
Member Author

OK here is the new home icon look:

screen shot 2014-02-05 at 3 36 11 pm

@fperez
Copy link
Member

fperez commented Feb 5, 2014

Love the home icon!

@ellisonbg
Copy link
Member Author

Ok I think this is ready to go unless there are more comments. Thanks for the review @minrk !

minrk added a commit that referenced this pull request Feb 6, 2014
Add directory navigation to dashboard
@minrk minrk merged commit c471ed3 into ipython:master Feb 6, 2014
@fperez
Copy link
Member

fperez commented Feb 6, 2014

Wohoo, awesome!! I'm truly thrilled, thanks for this.

@fperez
Copy link
Member

fperez commented Feb 7, 2014

I know this has been merged, but @ellisonbg asked me about the decision on window/page title (which appears in the tab name and window title for browsers that show that like Ffox/Safari). Do we want to push that to a new issue?

I honestly don't have hugely strong feelings on the matter, but I'm happy to help out finish up the discussion in a new issue if we want to feel like we've covered the main bases for the navigable dashboard.

@ellisonbg
Copy link
Member Author

Why don't we just live with it for a week and see how we feel about it. I don't think we need to open a new issue until we know we want to make a change.

@fperez
Copy link
Member

fperez commented Feb 7, 2014

+1

@ellisonbg ellisonbg deleted the dashboard-dirs branch February 8, 2014 19:53
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Add directory navigation to dashboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants