Dashboard "Running" Tab #5215
Dashboard "Running" Tab #5215
Conversation
|
Requisite screenshots: |
|
The wording when the list is empty ('actively running kernels') seems odd - I'd drop 'actively'. |
|
Looks good... a nice addition would be an open book as the icon, but font-awesome only have a close one... |
|
Ah...the reload button seems a little higher than the shutdown buttons... I think it will fit better if it has the same height... |
before this, kernel_list and notebook_list each fetched and held onto their own copy of the sessions.
|
Ok, this is finished. I've factored out the sessions_list so that sessions are fetched only once but used by both the Notebooks and the Running tabs. |
|
Very nice. Only one note: The header says
but the empty message says:
Presumably we should pick either 'notebooks' or 'kernels'. I don't have a strong preference for which, but it seems like we try to avoid using the term 'kernels' with novice users, in which case 'notebooks' is probably best. |
|
@damianavila the reload button is the same as for Clusters and Notebooks, so any CSS shenanigans needed to fix the slight inconsistencies are out-of-scope for this PR ;) @minrk yeah, that sounds right, fixed. |
|
@ivanov ok... no problem... we can deal with the css later 😉 |
|
The code looks superficially good to me, but my JS expertise is so limited that this shouldn't be considered a proper review. User experience and functionality-wise, this looks great. I think it does just enough for what we need in 2.0, and we can leave a bigger refactoring, with multi-notebook selection/actions and other such niceties for a later refactoring. If the JS code passes review, big +1 from me to merge. Many, many thanks @ivanov for pushing this, great work. It will be super useful. |
| }; | ||
|
|
||
| NotebookList.prototype.style = function () { | ||
| $('#notebook_toolbar').addClass('list_toolbar'); | ||
| $('#' + this.element_name + '_toolbar').addClass('list_toolbar'); | ||
| $('#drag_info').addClass('toolbar_info'); |
There was a problem hiding this comment.
This line is only applying the toolbar_info class to the version in the notebook list. In the kernel list, that class should be applied to #running_list_info. This is causing the info text of the kernel list to be too high.
There was a problem hiding this comment.
ah, thanks for that tip, Brian, I've renamed drag_info to notebook_list_infoand now apply the toolbar_info class to both notebook_list_info and running_list_info
|
OK I have looked at this and tested it interactively. Overall it looks great. There are a few minor inline changes I found though. Once they are done, ready for merge. |
|
Thanks @ellisonbg! @ivanov, I think with this much review, once you get those fixes in, you can go ahead and apply the merge. |
| success : $.proxy(that.sessions_loaded, this) | ||
| }; | ||
| var url = utils.url_join_encode(this.base_url, 'api/sessions'); | ||
| $.ajax(url,settings); |
|
Only some aesthetic comments... 😉 |
in the dashboard, I've renamed drag_info to notebook_list_info, so applying style to notebook_list_info and running_list_info can be done in one place.
|
alright, Travis is happy, and so am I. Thanks for the feedback on this one, everyone, merging! |
Dashboard "Running" Tab
this is following the discussion from #5140
I still need to refactor it a bit, but it mostly works now
Refactoring