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.

Sign up for GitHub

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

Jump to bottom

Table view - squashed #2052

Merged

monfera merged 44 commits into master from table-squashed

Oct 5, 2017

Contributor

monfera commented Oct 3, 2017

Squashed version of #1834

Before starting review, I'd push proper mocks, also as codepen examples. I'll also need to recheck attributes and add jasmine tests.


          table - initial commit (squashed feature branch)

c4244db

monfera self-assigned this

monfera added status: in progress feature labels

monfera added this to the v1.31.0 milestone


          adding birds mock

865e444

Contributor Author

monfera commented Oct 3, 2017 •

edited

Loading

@etpinard Bird mock is now added, I'll need to replace the other tech test mocks too (they have JS etc.) table_plain_birds.json

monfera requested a review from etpinard

October 3, 2017 14:13


          minor mock change

2648e51

etpinard reviewed

View reviewed changes

Contributor

etpinard left a comment

Here's a first review. My comments are mostly pointing at ways to reuse existing code especially components Drawing and Color. I caught a few typos in there too.

I'm a little disappointing that the new text-wrapping code is embedded deep inside table/plot.js. How hard would it be to factor it out and place it in svg_text_utils.js?

src/traces/table/attributes.js

+                      },
+                      format: {
+                          valType: 'data_array',

Contributor

etpinard Oct 3, 2017

Maybe this should be an arrayOk valType: 'string', to format all values to same way more easily?

src/traces/table/attributes.js

+                      height: {
+                          valType: 'number',
+                          dflt: 28,

Contributor

etpinard Oct 3, 2017

Interesting number. How did you come up with that?

Contributor Author

monfera Oct 3, 2017

Just took a look at what'd look balanced with a font height of 12px although it could as well be computed

src/traces/table/attributes.js

+                          }
+                      },
+                      font: {

Contributor

etpinard Oct 3, 2017

please use https://github.com/plotly/plotly.js/blob/master/src/plots/font_attributes.js

src/traces/table/attributes.js Outdated

+                          role: 'info',
+                          dflt: [],
+                          description: [
+                              'Dimension values. `values[n]` represents the value of the `n`th point in the dataset,',

Contributor

etpinard Oct 3, 2017

Shouldn't values be 2D? That is, values[n][m] represents the value of ....

Contributor Author

monfera Oct 3, 2017

I'll need to revise all attribute descriptions, indeed it's a nested array:

src/traces/table/attributes.js Outdated

+                          }
+                      }
+                  }
+              };

Contributor

etpinard Oct 3, 2017

You'll need to add editType declarations to these attributes.

src/traces/table/plot.js Outdated

+              function conditionalPanelRerender(gd, tableControlView, cellsColumnBlock, pages, prevPages, d, revolverIndex) {
+                  var shouldComponentUpdate = pages[revolverIndex] !== prevPages[revolverIndex];
+                  if(shouldComponentUpdate) {
+                      window.clearTimeout(d.currentRepaint[revolverIndex]);

Contributor

etpinard Oct 3, 2017

no need to spell out window when using setTimeout and clearTimeout

src/traces/table/plot.js

+                              // this code block is only invoked for items where d.cellHeightMayIncrease is truthy
+                              var element = this;
+                              var columnCellElement = element.parentNode;
+                              var box = columnCellElement.getBoundingClientRect();

Contributor

etpinard Oct 3, 2017

Can we avoid these getBoundingClientRect calls?

Collaborator

alexcjohnson Oct 3, 2017

The key test here (which I assume will fail given the getBoundingClientRect call) is to call Plotly.newPlot into an element that's not in the DOM, and insert it in the DOM afterward - ala https://codepen.io/anon/pen/XjakQy
If you really do need a getBoundingClientRect result, use Drawing.bBox.

Contributor Author

monfera Oct 3, 2017

I think that #2053 should address it. I added a 'core' line now to express this. Here, the difference between parent and child bounding box top matters. What do people, or we, do with detached nodes? I'm trying to minimize open points for this specific PR, unless of course users would run into serious issues with it otherwise.

src/traces/table/plot.js Outdated

+                          });
+                      // MathJax styling has to be killed for HTML DOM color specifications to seep through
+                      cellTextHolder.selectAll('svg').selectAll('*')

Contributor

etpinard Oct 3, 2017

Looks hacky. Can we do better?

Collaborator

alexcjohnson Oct 3, 2017

in regular convertToTspans MathJax gets colored here based on the color that was already applied to the text it was converted from - does that not apply here?

Contributor Author

monfera Oct 3, 2017

I'll check your link, maybe it solves the need for the current hack, I wasn't happy it drew in black when fill was white but thought maybe there's a better way.

Collaborator

alexcjohnson Oct 3, 2017

probably just an order-of-operations issue, we assume the text is fully styled before being converted to MathJax. It would probably be good for us to break this out like we did with positionText so it can be applied either before or after conversion.

Contributor Author

monfera Oct 3, 2017

Ah, thanks for the lead!

Contributor Author

monfera Oct 4, 2017 •

edited

Loading

@alexcjohnson @etpinard Fully agree w/ the order of operations (LaTeX coloring), a quick initial look didn't lead to a 5-minute solution, plan to do it tomorrow. Other tasks are,

ensuring table works fine with Plotly.restyle, and
more defaulting should happens (eg. now the user always needs to specify column order)

which seem more pressing than internal changes.

Contributor

etpinard Oct 5, 2017

Ok. Please add item to #2056

test/jasmine/tests/table_test.js Outdated

+                      it('\'dimension\' specification should have a default of an empty array', function() {
+                          var fullTrace = _supply({});
+                          expect(fullTrace.dimensions).toEqual([]);

Contributor

etpinard Oct 3, 2017

What is this? A copy from parcoords?

Contributor Author

monfera Oct 3, 2017

Yes, vestigial. I'll read through the code before the next round.

test/jasmine/tests/table_test.js Outdated

+                  });
+              });
+              describe('@noCI table', function() {

Contributor

etpinard Oct 3, 2017

Why @noCI?

Contributor Author

monfera Oct 3, 2017

Basically I didn't intend to check in table_test.js 😃

Contributor

etpinard commented Oct 3, 2017 •

edited

Loading

A few (opinionated) comments from https://codepen.io/monfera/pen/ZXXMLo?editors=1010#0

I find column swapping a little too bubbly:

Is there an easy way to reduce the recoil action on drag-end?

It would be nice to add plotly_hover and plotly_click events on the cells:
It would be nice to make the header values (and maybe even the cells?) editable: true

etpinard reviewed

View reviewed changes

test/image/mocks/table_plain_birds.json

    
                      "cells": {

                          "values": [

Contributor

etpinard Oct 3, 2017 •

edited

Loading

Hmm. So here the values[i] represent columns - which I think is fair for table. But this means that cells.values is the transpose of the z attribute in heatmap traces. Which at the very least write this down in the table/index.js module description.

Collaborator

alexcjohnson commented Oct 3, 2017

thinking ahead to Plotly.react, we'll probably need an attribute for scroll position, so the trace attributes fully specify the view. Also for export, people may want to be able to snapshot at the current view.

Contributor

rreusser commented Oct 3, 2017 •

edited

Loading

@alexcjohnson That's true. The only caveat is that high-frequency updates like scroll position shouldn't go through controlled update cycles or even setState (which is delayed by one frame and so makes things unnecessarily jumpy). In other words, I don't think the scroll position should be controlled. Which is equivalent to saying it would just be initialScrollY/initialScrollX or equivalent.

So in summary, it updates the plotly attr so the current scroll position is reproduced correctly, but after that it can only be set via the UI.

Glad to hear other thoughts on the matter, but that's what I think needs to happen.

Contributor Author

monfera commented Oct 3, 2017

@etpinard I can taper down the bouncy column drag to less or non-bouncy easing.

Contributor Author

monfera commented Oct 3, 2017 •

edited

Loading

@alexcjohnson @rreusser is it decided that plot state will be fully specified? That'd include things like zoom level, pan position, the use of a selection box or even the contours of a lasso. Not suggesting it's not a desired thing at all though!

@rreusser I'm not sure if it adds to or questions anything you said, but wanted to mention that controlling scroll position might be desirable. For example, some journal incorporates a table and links scroll position to an advance-based story arc, or part of some product tour. I think it'd apply to other things as well. I'd prefer observables over fully restating the state because it allows lightweight, minimal updates upon very specific signals, ie. you save on both spec diffing and incremental rerendering. Such signals may be a lot of things besides scroll position; could be for responsive features such as devicePixelRatio, window/document or container element size, or even things like mouse position (for hover).

Contributor

rreusser commented Oct 3, 2017 •

edited

Loading

@monfera That's valid. I was wondering about that. I guess the key part is that we somehow avoid feeding UI scrolling from feeding back into controlled UI updates.

Collaborator

alexcjohnson commented Oct 3, 2017 •

edited by monfera

Loading

is it decided that plot state will be fully specified? That'd include things like zoom level, pan position, the use of a selection box or even the contours of a lasso.

Yes. Specify all the things.

I guess the key part is that we somehow avoid feeding UI scrolling from feeding back into controlled UI updates.

We can certainly throttle these events (wooo we have a util for that 🎉 ) but we definitely want users to be able to watch the scroll state, and for example highlight points on a separate plot corresponding to the visible rows, and vice versa - click a point and scroll to that row in the table (hmm, those are a little more involved, as they require not only the pixel scroll, which is required to fully specify the state, but also some way to translate that bidirectionally to row numbers). Anyway my more immediate concern is other edits not resetting the table scroll - like the issues @etpinard fixed for geo plots in #2030.

@monfera edit here, as we don't have threading and topic locality is nice: added the need for full representation as an item to the follow-up issue #2056

Collaborator

alexcjohnson commented Oct 3, 2017

Looks to me like shape-rendering: crispEdges is getting inherited in a few places it shouldn't: the scroll handle and MathJax are where I notice it.

This was referenced Oct 3, 2017

Unification of text rendering and wrapping #2053

Closed

Consider and possibly put in place a common, unified scroller solution #2054

Closed

monfera added 3 commits

October 3, 2017 23:02


          avoid crispEdges in cell contents and scrollbar; minor mock change


          doc change

30cbaa0


          fewer accessor uses

aa0c116

monfera mentioned this pull request

Open items for future PRs related primarily to table #2056

Closed

21 tasks


          doc update

c0ac213

monfera mentioned this pull request

Centralize drag handlers #2057

Closed

monfera added 2 commits

October 3, 2017 23:23


          more idiomatic D3

732bd46


          no prefixing with window

22e23e3

monfera mentioned this pull request

Code style rework: class names; extracted utils #2058

Closed


          removed clipView debug option

2be591f

monfera force-pushed the table-squashed branch from 8ad24bf to 2be591f Compare

October 5, 2017 08:24


          allow cell text selection

addb85b

monfera force-pushed the table-squashed branch 4 times, most recently from 45b6843 to addb85b Compare

October 5, 2017 13:20


          fixing server image gen issue - thanks Étienne - and removing the pos…

c2f251f

…t-hoc LaTeX styling workaround

monfera force-pushed the table-squashed branch from 85fdc93 to c2f251f Compare

October 5, 2017 14:33

monfera and others added 14 commits

October 5, 2017 16:57


          adding baselines

3cab51d


          globally safe clipPath #1

fa84c68


          use overrideAll in table attributes

465b45b


          globally safe clipPath #2

0fa1cc8


          Merge branch 'table-squashed' of https://github.com/plotly/plotly.js …

cd0578d

…into table-squashed


          add baselines

a6c4935


          preparing for user-specified stroke/stroke-opacity, fill/fill-opacity…

0ad1a0c

… setting via Drawing.*


          user-specified stroke/stroke-opacity, fill/fill-opacity setting via C…

1254c9f

…olor.*


          Merge branch 'table-squashed' of https://github.com/plotly/plotly.js …

e38b7cf

…into table-squashed


          Merge branch 'table-textselect' into table-squashed

94338f4


          minor simplification before rewiring event handlers to enable text copy


          allow both dragging and text cell copying

154ab24


          disable visible scrollbar on initial load


          updating other mocks

d4f0ed7

Contributor

etpinard commented Oct 5, 2017 •

edited

Loading

This sums up this PR. A very nice looking and performant trace type. 👏 to @monfera for his very user-first approach. But, unfortunately, not much work was put in reusability for our other components.

I hope @monfera will have time to make a followup PR knocking out a few items in #2056 in the next few days and adding jasmine tests along the way. Factoring out the text wrap logic ( #2053 ) we'll make a bunch of folks happy, I hope this happens soon. Items #2057 and #2054 are less important to me in the short-term.

Let's get this thing in master 💃 💃

This was referenced Oct 5, 2017

Provide a data table view usable via JavaScript #991

Closed

[WIP] Table view #1834

Closed

Contributor Author

monfera commented Oct 5, 2017

Thanks @etpinard and @alexcjohnson for the numerous review points!

monfera changed the title ~~[WIP] table view - squashed~~ Table view - squashed

monfera merged commit 50e933c into master

alexcjohnson deleted the table-squashed branch

October 5, 2017 19:45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature