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

Improvements to table: restyle fix, 0 row/column and misc. fixes; jasmine tests #2107

Merged
merged 7 commits into from
Oct 20, 2017

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 19, 2017

  • restyle fix was urgent due to Restyling table traces #2092
  • why not take the opportunity to add jasmine coverage for most of what exists
  • tests uncovered misc. special case issues which were fixed

@monfera monfera self-assigned this Oct 19, 2017
@monfera monfera added bug something broken type: maintenance labels Oct 19, 2017
@VeraZab
Copy link

VeraZab commented Oct 20, 2017

I'll test out this branch on streambed, see if the restyle calls I need work now, thanks!

@VeraZab
Copy link

VeraZab commented Oct 20, 2017

ok, I see the improvements,

here's a few observations:

  1. there's inconsistency where I I need to wrap my data into an extra set of arrays to make some calls work:
    for example: https://codepen.io/veraz/pen/EwMLBQ?editors=1010
    I can make Plotly.restyle('graphDiv', {'header.values': [[['A'], ['B'], ['C']]]}) work, with one extra set of arrays. It's the same for a few other calls like: Plotly.restyle(gd, {'cells.font.size': [[16, 5]]})

  2. A restyle to cells.fill.color behaves unexpectedly, it should color by row right? not by column..

I'd really like to see that extra [] wrapping fixed in this pr, would really help for the workspace integration.

Thanks!

});

it('Calling `Plotly.relayout` with string should amend the preexisting parcoords', function(done) {
expect(gd.layout.width).toEqual(1000);
Copy link

Choose a reason for hiding this comment

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

preexisting 'tables' instead of 'parcoords'

});

it('Calling `Plotly.relayout` with object should amend the preexisting parcoords', function(done) {
expect(gd.layout.width).toEqual(1000);
Copy link

Choose a reason for hiding this comment

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

can we add a few tests testing some of those array attributes?

@monfera
Copy link
Contributor Author

monfera commented Oct 20, 2017

Hi @VeraZab yes #1 is the way plotly.js works when it's an array attribute of traces (of which there can be multiple). Re #2, it can do per column, per row, per cell or combination. The idea is that if the columns in a three-column table are [A, B, C], then eg. B can be a scalar value or color; but it can also be an array. So you can have 3 columns, 2 rows styled as [[A1, A2], [B1, B2], [C1, C2]] on a per cell basis, or you can do just [[A1, A2]] ie. leave other columns undefined, in which case they inherit colors, so it's basically per-row styling. I'll make an example.

@monfera
Copy link
Contributor Author

monfera commented Oct 20, 2017

Per column: as you did above
Per row: https://codepen.io/anon/pen/vePreY?editors=1010
Combined: https://codepen.io/anon/pen/XeGYzB?editors=1010

@monfera
Copy link
Contributor Author

monfera commented Oct 20, 2017

@VeraZab this version replaces cell contents in one call, ie. not of just one column at a time: https://codepen.io/anon/pen/dVrKqb?editors=1010 - I'm adding jasmine test cases for such things, and what you're pointing out above as well.

@VeraZab
Copy link

VeraZab commented Oct 20, 2017

oh for #1, yeah sorry there's a bunch of things that I'm realizing are not working as expected in the workspace, so I was making the calls from the workspace console to test your pr, rather from the workspace ui, which already has those conventions pre-built, yeah I think that's fine.

@VeraZab this version replaces cell contents in one call, ie. not of just one column at a time: https://codepen.io/anon/pen/dVrKqb?editors=1010 - I'm adding jasmine test cases for such things, and what you're pointing out above as well.

yeah, both are supported right, from what I've seen in my tests, and that's great, should be that way, and yes, let's add some tests

Ok, well, aside from a few extra tests, I'm good with this pr 💃

@etpinard
Copy link
Contributor

Oh shit, what Vera points out will be very tricky to fix. We can't rely on what we currently do for other 2d array trace types as table is column major.

@VeraZab
Copy link

VeraZab commented Oct 20, 2017

@etpinard I think I've made a mistake in that comment, about array wrapping.

I've just checked for a scatter trace, and it's true that when I want to send an update to marker.color, I don't just do Plotly.restyle(gd, {'marker.color': [1, 2, 3]})

I wrap the [1, 2, 3] array into another array and the correct call is
Plotly.restyle(gd, {'marker.color': [[1, 2, 3]]})

so my bad, I think this pr's good to go, after tests

@monfera monfera merged commit c69aeb1 into master Oct 20, 2017
@monfera monfera deleted the table-restyle-squashed branch October 20, 2017 21:54
@etpinard etpinard mentioned this pull request Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants