[Issue 203] Add report dashboard layout #211

dalogsdon · 2016-04-04T16:06:49Z

Resolves #203

parente · 2016-04-04T16:18:18Z

jupyter_dashboards/nbextension/notebook/dashboard-view/dashboard-metadata.js

+                }
+            }
+        }
+**************/


👍 for moving all the metadata business to its own module.

parente · 2016-04-04T18:40:14Z

The "Hidden Cells" annotation in the UI seems to have disappeared in grid layout mode:

parente · 2016-04-04T18:42:33Z

When taking an existing notebook and going from grid layout to report layout, the spacing between cell rows goes crazy. Try it on the scotch notebook.

Also: it seems mighty dangerous that a single misclick will potentially lose a complete dashboard layout when the user accidentally switches modes.

parente · 2016-04-04T18:53:58Z

jupyter_dashboards/nbextension/notebook/dashboard-view/dashboard-actions.js

-        scrollToBottom = IPython.Notebook.prototype.scroll_to_bottom;
-        IPython.Notebook.prototype.scroll_to_bottom = function() {};
+    function getFaIconClass(el) {
+        return $(el).attr('class').split(' ').filter(function(value) {


dashboard-actions.js?v=20160404185313:55 Uncaught TypeError: Cannot read property 'split' of undefined

When opening the scotch-demo dashboard for the first time with these changes.

dalogsdon · 2016-04-04T18:57:49Z

The "Hidden Cells" annotation in the UI seems to have disappeared in grid layout mode

I was seeing the label during development. I did a fix this morning for a minor issue I saw, maybe that broke it. l'll have a look.

When taking an existing notebook and going from grid layout to report layout, the spacing between cell rows goes crazy.

Not sure what you mean by goes crazy. When doing the transition myself, I didn't notice anything unusual. I'll try with scotch notebook.

it seems mighty dangerous that a single misclick will potentially lose a complete dashboard layout when the user accidentally switches modes

That is why I put the layout selection in a dropdown menu. It is an attempt to make the action to switch layout more intentional. If you feel it is still too easy, we could experiment with a confirmation, though I was hoping to avoid any popup dialogs.

I've also just realized I put the toolbar layout selection in a dropdown but kept the menu items flat. I could also put the layout selection in a sub-menu to help mitigate any accidental layout wipeouts.

parente · 2016-04-04T19:03:00Z

Not sure what you mean by goes crazy. When doing the transition myself, I didn't notice anything unusual. I'll try with scotch notebook.

There's huge amounts of cell spacing between the cells when flipping the notebook between modes. A refresh clears things up. I bet the grid is not being computed correctly.

hoping to avoid any popup dialogs.

Definitely do NOT want this. I'm more thinking about how do we set user expectations that once they switch, the layout is lost. They only way to find out that hard truth at the moment is to try it.

dalogsdon · 2016-04-04T19:38:19Z

There is an issue with switching to report layout with pre-existing hidden cells. This seems to cause both the missing hidden label and the spacing issue.

dalogsdon · 2016-04-04T19:40:18Z

An alternative to the lost layout issue is to retain all layout metadata, so the user can freely move between layouts as desired. Not sure if this would be confusing or not.

dalogsdon · 2016-04-04T19:55:45Z

Ah, I put some debounced code to show the hidden area after the Gridstack is fully built, but that only happens if there are cells with no grid metadata in which case we add a widget which triggers the grid change event. If the notebook already has the layout metadata this code will not fire. I'll have to redo when the hidden area is recomputed to handle this case.

parente · 2016-04-04T20:07:51Z

An alternative to the lost layout issue is to retain all layout metadata, so the user can freely move between layouts as desired. Not sure if this would be confusing or not.

The problem with this is that the user would have a hard time knowing which is active. "The last one you viewed in layout mode" is a pretty weak indicator.

I think overwriting is fine for now. When we do the metadata proposal for jupyter/roadmap#8, we can consider multiple named layouts.

dalogsdon · 2016-04-04T20:45:17Z

Another hidden-cell related issue is that I was wiping out hidden data when switching to report layout. I've updated the metadata module to respect hidden cells when updating layout for all cells.

@parente With the fixes I've done for hidden cells, the scotch demo looks correct now.

parente · 2016-04-04T23:30:45Z

OK I'll give it another shot.

parente · 2016-04-05T11:55:24Z

Notebook with no dashboard layout yet results in Uncaught TypeError: Cannot read property 'dashboard' of undefined from :9500/nbextensions/jupyter_dashboards/notebook/dashboard-view/dashboard-metadata.js?v=20160405115407:109

Steps to reproduce:

Open got_scotch_demo/scotch_analysis.ipynb
Click layout dropdown button.
Click report layout.

parente · 2016-04-05T12:02:47Z

Still getting bad spacing / layout in certain situations:

Create a new notebook
Enter and run x = 1, y = 2, x + y in 3 different cells.
Click grid layout button (not the dropdown since that results in the bug in my previous comment)
Grid layout looks OK.
Switch it to the report layout.

Hidden cells text disappears. The one lone cell is moved way down the page with too much whitespace around it. (Layout is fubar.)

parente · 2016-04-05T12:03:45Z

Marked WIP so that we know there's issues still being worked here.

jhpedemonte · 2016-04-05T16:41:38Z

jupyter_dashboards/nbextension/notebook/dashboard-view/dashboard-actions.js

-            .addClass('active')
-            .siblings().removeClass('active');
+    function updateAuthoringBtn(state) {
+        if (/^db-auth-/.test(state)) {


If we're setting up (basically) an ENUM at the beginning of the file (var STATE = {...}), I think we should use that everywhere, rather than sometimes also using the values. I know it will be more verbose, but it will make it easier to maintain and search in the future.

If you really need to map from values to enum, that should be refactored into its own getter function, preferably right below the definition of the enum itself.

(Here and elsewhere).

I'll probably just remove the db-auth- prefix since it introduces some needless complexity. The association of the states with the button group is done in the template anyway, and it will probably still make sense to just use 'grid' and 'report' for the dashboard UI states.

dalogsdon · 2016-04-06T20:06:46Z

I merged in the grid move buttons and updated based on suggestions from @jhpedemonte.

@parente It looks like the metadata wasn't getting fully saved when switching between layouts. I've updated how the metadata is saved so your issue should be fixed.

dalogsdon · 2016-04-06T21:59:41Z

I just noticed an issue related to the getter/setter updating I did earlier. It is trying set the layout before the metadata is initialized. I've updated to fix.

This was causing 2 errors on a new notebook: immediate error on page load and error when trying to load report layout as the first dashboard layout.

parente · 2016-04-07T11:51:46Z

Testing on 2016-04-07:

I think we have a design flaw stemming from relying on the grid-layout mechanism for report mode.

In grid layout, the user gets to pick the height for a cell and we rely on him/her to set it big enough for whatever dynamic content a cell might have. In report layout, we take away that option because we assume the user just wants to see the cells top to bottom just like in the notebook. The problem is, we're still using absolute positioning, so the user can wind up with a notebook like this:

All I did was render a big table that is too big for the default height. I can't correct the problem unless I go to grid mode, set a height, then come back to report mode, but that's a hack. We could enable vertical sizing for cells in report mode, but then I think we've defeated the purpose of report mode entirely which is supposed to be as simple as "just let me pick which cells to show / hide".

Most of the other cell height problems below stem from this problem too.

One inconsistency that I think should get fixed: When I'm in report mode, the close X is in the top right of the cell:

In grid mode, the X shifts to the left of the move buttons:

I'm trained to look for close either in the top right or top left of a window, so I think it should stay put.

Spacing for a 1 line markdown cell is huge in the test_layout_basic dashboard when going from grid to report. Looks like height is not recomputed properly.

Test layout has a similar problem. Plus, the first cell is too short and clips the markdown text.

Hidden cells header text missing again in test_html_javascript. Just go to grid mode to (not) see it.

Occurs to me that we also need to update our old Thebe-based "Local deploy" template logic to support report mode. It may "just work" like it says in jupyter/dashboards_server#157, but I've opened jupyter/dashboards_bundlers#25 to for testing to make sure it does.

dalogsdon · 2016-04-07T14:49:03Z

We could enable vertical sizing for cells in report mode

It should be enabled. I tried it just now and it worked. Perhaps the cell content was blocking the resize handle?

but then I think we've defeated the purpose of report mode entirely which is supposed to be as simple as "just let me pick which cells to show / hide"

Yes, that makes sense. We could override the CSS or create a new very simple layout module for report.

One inconsistency that I think should get fixed: When I'm in report mode, the close X is in the top right of the cell

Good catch. This was fixed in #208 but then regressed when merging with #203 since the grid controls template was moved from inline string to a file. I can fix that.

I'll debug into the other issues you mentioned.

parente · 2016-04-14T12:19:14Z

I've realized that this PR now contains more additions and deletions than the sum of all my previous commits.

You're going to win! 🏆

parente · 2016-04-14T12:34:35Z

I gotta do a bit more testing with the decl widgets to see how they behave in the DOM flow, but as far as the collapse/expand UX goes, count me in. I can definitely get a sense of what the entire thing will look like by collapsing the contiguous cells. And it's not too hard to expand, find a cell, add it, and re-collapse when I move through the notebook.

I tested a bunch of edge cases too like hiding a set of cells, collapsing them as a group, hiding another set of cells with a shown cell between the groups, and then hiding that one shown cell to make sure the two collapse groups join. They do. Nice work.

jtyberg · 2016-04-14T16:43:47Z

Oh, yes. This works for me.

I only have one comment:

When I unhide a cell, I want to see it full height. Currently it shows me a fixed height.

… Gridstack. (c) Copyright IBM Corp. 2016

dalogsdon · 2016-04-14T20:00:37Z

When I unhide a cell, I want to see it full height. Currently it shows me a fixed height.

Good catch. That is a styling bug that happens when switching to the report layout before running the cells. I've updated to fix.

(c) Copyright IBM Corp. 2016

dalogsdon · 2016-04-14T21:09:52Z

I found one other issue. We have several HTML template files we add to the page. These have the Jupyter copyright headers in them, so we were adding copyright header comments to the document for each template. This PR made this issue more visible, so I've included a simple fix here, which is a requirejs plugin called "template" that will read a template file and remove any comment nodes.

parente · 2016-04-14T22:28:32Z

It's also reasonable to just leave the Jupyter copyright out of the templates.

parente · 2016-04-15T12:16:23Z

I tested with declarative widgets 0.4.5 by opening the streaming meetup demo, starting it running, then manipulating the cells visible in the report layout. No issues throughout.

I say

One final note: I'm on a desktop w/ monitor today and noticed it was a bit harder to quickly make out the difference between hidden and shown cells when not collapsed. I don't want to hold this up any longer, but let's keep it in mind as something to tweak maybe if others report it.

parente · 2016-04-18T15:23:29Z

Make sure that if an older notebook has a grid layout without declaring as a grid layout, that it still shows properly when immediately opened in preview mode for backward compat.

dalogsdon · 2016-04-18T18:24:22Z

dashboard-metadata.js will return grid when accessing dashboardLayout by default. This will cause a notebook that has grid metadata without declaring grid layout to render properly and will cause a default grid layout to be set if the notebook has no dashboard metadata. However, the layout will not be set in either of these cases, so these notebooks will still rely on the default layout being grid. I could update this so preview mode sets the layout to grid if not set.

jhpedemonte · 2016-04-18T18:50:11Z

jupyter_dashboards/nbextension/notebook/dashboard-view/dashboard-actions.js

+
+    function updateUIState(state) {
+        updateUrlState(state === STATE.DASHBOARD_PREVIEW);
+        setHeaderVisibility(state !== STATE.DASHBOARD_PREVIEW);


Pull out these equality checks into their own variable? May make it easier to read this function and avoid doing same call multiple times:

var inDashboardPreview = state === STATE.DASHBOARD_PREVIEW;

dalogsdon · 2016-04-18T22:31:17Z

Updated based on comments by @jhpedemonte

parente · 2016-04-19T01:58:37Z

As soon as I open any notebook now, the header and toolbars are immediately hidden. In notebook mode I mean.

parente · 2016-04-19T02:07:19Z

jupyter_dashboards/nbextension/notebook/dashboard-view/dashboard-actions.js

+    function updateUIState(state) {
+        var isDashboardPreview = state === STATE.DASHBOARD_PREVIEW;
+        updateUrlState(isDashboardPreview);
+        setHeaderVisibility(isDashboardPreview);


Parameter needs to be negated.

parente · 2016-04-19T02:54:38Z

I'll submit a PR to fix the toolbar state as a follow-on. This one has gone on long enough.

parente reviewed Apr 4, 2016
View reviewed changes

dalogsdon force-pushed the 203-report-layout-2 branch from 4fe7e3e to 35e7a1b Compare April 4, 2016 20:44

parente changed the title ~~[Issue 203] Add report dashboard layout~~ [WIP] [Issue 203] Add report dashboard layout Apr 5, 2016

parente mentioned this pull request Apr 5, 2016

Bump dev image version to notebook 4.1 #213

Merged

jhpedemonte reviewed Apr 5, 2016
View reviewed changes

dalogsdon force-pushed the 203-report-layout-2 branch 3 times, most recently from a57b8e1 to 9077e66 Compare April 6, 2016 19:59

dalogsdon force-pushed the 203-report-layout-2 branch from 9077e66 to 88a176a Compare April 6, 2016 21:58

[Issue 203] Move report layout as a separate layout that does not use…

1f92e0a

… Gridstack. (c) Copyright IBM Corp. 2016

dalogsdon force-pushed the 203-report-layout-2 branch from dd9a384 to 1f92e0a Compare April 14, 2016 19:59

[Issue 203] Load templates without comment nodes

48cb0b8

(c) Copyright IBM Corp. 2016

dalogsdon changed the title ~~[WIP] [Issue 203] Add report dashboard layout~~ [Issue 203] Add report dashboard layout Apr 18, 2016

jhpedemonte reviewed Apr 18, 2016
View reviewed changes

parente mentioned this pull request Apr 18, 2016

[Issue 157] Support report layout jupyter/dashboards_server#190

Merged

[Issue 203] Preview dashboard as grid when layout isn't set

b1c04d5

Includes case where no dashboard metadata is set (c) Copyright IBM Corp. 2016

dalogsdon force-pushed the 203-report-layout-2 branch from 36082e7 to b1c04d5 Compare April 18, 2016 20:23

[Issue 203] Report layout code cleanup

03b3289

(c) Copyright IBM Corp. 2016

parente reviewed Apr 19, 2016
View reviewed changes

parente merged commit 7cbbf22 into jupyter:master Apr 19, 2016

parente mentioned this pull request Apr 19, 2016

Fix state tracking for header and toolbar #220

Merged

[Issue 203] Add report dashboard layout #211

[Issue 203] Add report dashboard layout #211

Conversation

dalogsdon commented Apr 4, 2016

parente Apr 4, 2016

Choose a reason for hiding this comment

parente commented Apr 4, 2016

parente commented Apr 4, 2016

parente Apr 4, 2016

Choose a reason for hiding this comment

dalogsdon commented Apr 4, 2016

parente commented Apr 4, 2016

dalogsdon commented Apr 4, 2016

dalogsdon commented Apr 4, 2016

dalogsdon commented Apr 4, 2016

parente commented Apr 4, 2016

dalogsdon commented Apr 4, 2016

parente commented Apr 4, 2016

parente commented Apr 5, 2016

parente commented Apr 5, 2016

parente commented Apr 5, 2016

jhpedemonte Apr 5, 2016

Choose a reason for hiding this comment

dalogsdon Apr 5, 2016

Choose a reason for hiding this comment

dalogsdon commented Apr 6, 2016

dalogsdon commented Apr 6, 2016

parente commented Apr 7, 2016

dalogsdon commented Apr 7, 2016

parente commented Apr 14, 2016

parente commented Apr 14, 2016

jtyberg commented Apr 14, 2016

dalogsdon commented Apr 14, 2016 • edited Loading

dalogsdon commented Apr 14, 2016

parente commented Apr 14, 2016

parente commented Apr 15, 2016

parente commented Apr 18, 2016 • edited by dalogsdon Loading

dalogsdon commented Apr 18, 2016

jhpedemonte Apr 18, 2016

Choose a reason for hiding this comment

dalogsdon commented Apr 18, 2016

parente commented Apr 19, 2016 • edited Loading

parente Apr 19, 2016

Choose a reason for hiding this comment

parente commented Apr 19, 2016

dalogsdon commented Apr 14, 2016 •

edited

Loading

parente commented Apr 18, 2016 •

edited by dalogsdon

Loading

parente commented Apr 19, 2016 •

edited

Loading