Table values may be left unspecified

[monfera]

Fixes #2312

@alexcjohnson if I remember, last time we needed to increase flexibility in the face of missing or partial input, you suggested it's better to do it in the "userland" code than in table/defaults.js, is it the legit thing to do here as well?

@VeraZab it fixes this specific usage, are there perhaps similar paths that should be addressed in table, or was it just this one?

[alexcjohnson]

Yes, this is a good way to handle it. Are there any other attributes that might cause problems if they're missing? Is it OK to have cells with no header?

Can you add a test for this? Just that no errors are thrown and the headers are displayed?

[monfera]

@alexcjohnson yes, planning to add a test, awaiting the response to your question which was essentially my question too to @VeraZab. On cell contents without header or partial header: only as many columns are rendered as the number of header items, so it'd render nothing if unsupplied, no error raised, which isn't terribly useful. If wanted, we could fill up with the empty string but then there'd be a header row with blank cells, not that good. So it's best to decide if there's a need to support tables with no header, a rare use case but maybe needed with an incremental table build (in which case a blank header row temporarily may be more intuitive than no header row).

[VeraZab]

I believe it's the only one I've seen for now, thanks!

[alexcjohnson]

I definitely think we should be able to show a table with no header - I don't know why people would do it, but I'm sure they will at some point 😈

Ideally I feel like it should not even show the header, just the cells, though since the header also has a UI function that we don't want to lose, perhaps we could just make it blank and reduce its height to something like 5 px?

[monfera]

Thanks @alexcjohnson I'll make this change, as it's almost 11pm here I'd tweak it 1st thing tomorrow. If the present fix is urgent (@VeraZab ?), we could consider it for merging, because this other change is easy to do in a separate PR, no implementational bond between the two things.

[alexcjohnson]

Missing headers can certainly be handled in a separate PR, up to you whether to do that here or separately. I do think we want that done before we roll out the new editor to the workspace, but aside from that there's probably no rush.

[VeraZab]

There's no big rush, but I'm planning on having the editor support all chart types by the end of next week if possible, and then move on to other more complex workspace features.

So it'd be nice to have these table fixes in the next plotly.js release, when are you planning your next release @etpinard ?

[etpinard]

So it'd be nice to have these table fixes in the next plotly.js release

I'd like to release 1.34.0 early next week if possible.

[monfera]

Here's a default header for the dragging interactions. If the approach looks good, please tell me and I'll add a test case for it.

[alexcjohnson]

Here's a default header for the dragging interactions. If the approach looks good, please tell me and I'll add a test case for it.

😍

[monfera]

Tests are in; getting unrelated Jasmine test fails here, even after having rebased to current master.

[monfera]

... interestingly, CI test run restarts didn't solve the jasmine issue, but a dummy push -f with a minor comment change on the last commit did.

[alexcjohnson]

How did these changes to package-lock get here? Looks like just a different hash scheme but otherwise presumably equivalent... how can we avoid this? (cc @etpinard )

[etpinard]

Hmm that's strange. @monfera which node and npm versions are you using?

[monfera]

@alexcjohnson @etpinard I'm on npm 5.6.0 and node 9.3.0 but had no intention of checking in the package-lock.json, removed before the merge.

[etpinard]

Interesting. Maybe node 9.x uses a different hashing scheme.

[monfera]

Looks like various things may cause a flip to sha1, discussed eg. here

[alexcjohnson]

💃 once we figure out package-lock. Very nice test cases!