Row IDs by alexcjohnson · Pull Request #412 · plotly/dash-table

alexcjohnson · 2019-04-21T23:10:47Z

Row ID support - closes #312

data - each row can have id key, used as row id (can also be displayed if there’s a column with id='id')
active_cell -> {row, column, row_id, column_id}
selected_cells -> array like active_cell
selected_rows -> added selected_row_ids
start_cell and end_cell -> these actually didn't seem to be wired up? Now they look like active_cell
derived -> added
- derived_viewport_row_ids
- derived_virtual_row_ids
- derived_virtual_selected_row_ids
- derived_viewport_selected_row_ids

Still need to add tests, but AFAICT it works as intended.

Simple test app with pagination (so at least viewport indices are different) and row selection & deletion:

import json
import pandas as pd

import dash
import dash_html_components as html
from dash_table import DataTable
from dash.dependencies import Input, Output

df = pd.read_csv('https://raw.githubusercontent.com/plotly/datasets/master/solar.csv')
data = df.to_dict("rows")
for row in data:
    row['id'] = 'id:' + row['State'].lower()

app = dash.Dash(__name__)
app.layout = html.Div([
    DataTable(
        id='main',
        columns=[{'name': i, 'id': i} for i in df.columns],
        data=data,
        row_selectable='multi',
        row_deletable=True,
        pagination_settings={'page_size': 3, 'current_page': 0}
    ),
    html.Hr(),
    DataTable(
        id='out',
        columns=[{'name': 'prop', 'id': 'prop'}, {'name': 'val', 'id': 'val'}],
        data=[],
        style_cell_conditional=[
            {'if': {'column_id': 'val'}, 'textAlign': 'left'}
        ]
    )
])


props = [
    'active_cell', 'start_cell', 'end_cell', 'selected_cells',
    'selected_rows', 'selected_row_ids',
    'derived_viewport_indices', 'derived_viewport_row_ids',
    'derived_virtual_indices', 'derived_virtual_row_ids',
    'derived_viewport_selected_rows', 'derived_viewport_selected_row_ids',
    'derived_virtual_selected_rows', 'derived_virtual_selected_row_ids'
]


@app.callback(Output('out', 'data'), [Input('main', v) for v in props])
def show_props(*args):
    return [
        {'prop': prop, 'val': json.dumps(val)}
        for prop, val in zip(props, args)
    ]

if __name__ == '__main__':
    app.run_server(debug=True)

I was a bit schizophrenic about this before...

chriddyp · 2019-04-22T03:41:20Z

🌟 Looks pretty clean!

alexcjohnson · 2019-04-22T04:50:26Z

dash_table/Format.py

@@ -1,6 +1,4 @@
 import collections
-import inspect
-import sys


dunno why, but locally npm run lint asked me to make the changes in this commit, even though on CI it passes 🤔

Running the same versions? I've found that the default rules are updated frequently..

oh actually npm run lint excluded a whole bunch of rules that we really don't need to exclude (as long as we're excluding the auto-generated files, which we are). I'll un-exclude them. Probably worth taking a look at this in other repos as well... it makes sense to have looser linting in tests but some of these rules are useful - like unused imports, can improve perf, only files setting up namespaces have any reason to include them.

stricter linting -> 8f01c25

alexcjohnson · 2019-04-22T05:10:53Z

src/dash-table/components/ControlledTable/index.tsx

+        let endCol = end_cell && end_cell.column;
+
+        if (selectingDown) {
+            if (active_cell.row > minRow) {


I changed the logic here to be explicitly rectangular - ie picking out the region edges, editing these, and then recreating the selected_cells list from that at the end - rather than directly manipulating selected_cells as you had previously. I did this because it seemed both clearer and faster, I needed the new edges anyway for end_cell, and this way selected_cells will always be ordered as it appears on screen.

I considered changing these conditions from active_cell to start_cell. These are only different if you move active_cell after making the selection (via tab or enter), and as it stands some weird things can happen to the selection if you alter it via shift-arrow at that point - it's hard to predict whether shift-down will add a row at the bottom or remove one at the top. If I used start_cell instead of active_cell here, the cell you first clicked on would always be a corner of the selection, and shift-arrow would move the opposite corner. But there are two problems with that approach: (1) after you've moved active_cell, there's no visual indicator of which corner you started on, and (2) it's possible to shrink the selection so active_cell is no longer part of the selection. So in the end I left it with active_cell, but we could revisit that.

alexcjohnson · 2019-04-22T05:12:17Z

src/dash-table/components/ControlledTable/index.tsx

+        const newStartCol = endCol === minCol ? maxCol : minCol;
+
+        if (startRow !== newStartRow || startCol !== newStartCol) {
+            newProps.start_cell = makeCell(


updating start_cell only for that strange situation of reshaping the selection after moving active_cell

alexcjohnson · 2019-04-22T05:27:52Z

src/dash-table/derived/cell/operations.tsx

+        newProps.selected_rows = R.map(
+            // all rows past idx have now lost one from their index
+            i => i > idx ? i - 1 : i,
+            R.without([idx], selectedRows)


This was a bug previously - delete a row and any selected rows after it kept the same index, not the same row contents.

alexcjohnson · 2019-04-22T05:31:27Z

src/dash-table/derived/cell/operations.tsx

-        data: R.remove(idx, 1, data)
+        data: R.remove(idx, 1, data),
+        // We could try to adjust selection, but there are lots of edge cases
+        ...clearSelection


previously we left selected_cells intact when deleting a row, and only cleared active_cell if it was actually deleted. But that has some problems, and for simplicity and symmetry with deleteColumns I decided to just clear the selection here too.

alexcjohnson · 2019-04-22T05:41:34Z

src/dash-table/derived/paginator.ts

        loadNext: () => {
            pagination_settings.current_page++;
-            setProps({ pagination_settings });
+            setProps({ pagination_settings, ...clearSelection });


Clearing cell selection on page changes. You can select rows on separate pages and have them stay selected... but that's with row-by-row checkboxes, an inherently stickier interaction that feels right to persist even when a checked box is on a hidden page.

alexcjohnson · 2019-04-22T05:45:21Z

src/dash-table/handlers/cellEvents.ts

+     * And when you've selected multiple cells the browser selection is
+     * completely wrong.
+     */
+    window.getSelection().removeAllRanges();


For example, shift-click to select in the first example at https://dash.plot.ly/datatable:

alexcjohnson · 2019-04-22T05:49:12Z

src/dash-table/handlers/cellEvents.ts

@@ -1,57 +1,69 @@
-import * as R from 'ramda';
-import { SelectedCells, ICellFactoryProps } from 'dash-table/components/Table/props';
+import { min, max, set, lensPath } from 'ramda';


I made a couple of forays into specific ramda imports - my local linter complains about * as R and as @chriddyp pointed out (somewhere?) we may get a lighter build that way too.

Yes. Depending on your linter / editor's capacity to handle tsconfig, it may or may not handle the typescript option for * as R. This is allowed because of "allowSyntheticDefaultImports": true, in tsconfig.base.json.
The way imports are used here webpack should prune the unused content automagically but this is fine too.

alexcjohnson · 2019-04-22T05:53:35Z

src/dash-table/handlers/cellEvents.ts

+                minRow: min(row, start_cell.row),
+                maxRow: max(row, start_cell.row),
+                minCol: min(col, start_cell.column),
+                maxCol: max(col, start_cell.column)


Oh hmm, here I used start_cell and not active_cell like I did in the keypress handler... and indeed it can have weird effects like active_cell becoming outside the selection. So one of these should change... which one?

standardized on active_cell like keypresses #412 (comment) -> 1c69cfb

alexcjohnson · 2019-04-22T05:55:42Z

src/dash-table/handlers/cellEvents.ts

    // don't update if already selected
    if (selected) {
        return;
    }


BTW in case of shift-click I think this return is problematic - it means you can't shrink a selection without doing something hacky like first clicking over to the other side of the starting point.

better shift-click behavior -> 1c69cfb (and 3e31a7f)

Marc-Andre-Rivet · 2019-04-23T15:29:59Z

src/dash-table/dash/DataTable.js

+     * The row and column indices and IDs of the currently active cell.
     */
-    active_cell: PropTypes.array,
+    active_cell: cellCoordsIDsProp,


Assuming the resulting metadata.json is correct b/c the value is defined in the same file?

oh good call... looks like I can't 🌴 this in the existing react-docgen implementation ☹️

back to explicit prop types -> 397b914

Marc-Andre-Rivet · 2019-04-23T19:38:32Z

tests/visual/percy-storybook/DashTable.percy.tsx

    />));

-const columns = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']
+const columnsA2J = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']


Does A2J stand for Alex Johnson was here? 🤔

haha I almost called it columnsAJ but thought that might be too selfish. It's supposed to be just 'a' -> 'j'

alexcjohnson · 2019-04-25T00:47:15Z

@Marc-Andre-Rivet ready for review. I didn't change sorting_settings (#412 (comment)), would be happy to do that here or in a separate PR.

Marc-Andre-Rivet · 2019-04-26T14:10:32Z

package.json

    "private::host_js": "http-server ./dash_table -c-1 --silent",
    "private::lint:ts": "tslint '{src,demo,tests}/**/*.{js,ts,tsx}' --exclude '**/@Types/*.*'",
-    "private::lint:py": "flake8 --exclude=DataTable.py,__init__.py,_imports_.py --ignore=E501,F401,F841,F811,F821 dash_table",
+    "private::lint:py": "flake8 --exclude=DataTable.py,__init__.py,_imports_.py dash_table",


Thanks for cleaning that up / removing the exceptions

Marc-Andre-Rivet · 2019-04-26T14:11:53Z

src/dash-table/components/ControlledTable/index.tsx

+            // there should always be an active_cell if we got here...
+            // but if for some reason there isn't, bail out rather than
+            // doing something unexpected
+            return;


If this is unexpected, maybe add a Logger.warning here

Marc-Andre-Rivet

This looks pretty good to me 💃 Thanks for all the additional clean up.

alexcjohnson added 3 commits April 21, 2019 09:05

lint Format.py

3065280

remove obsolete comment about #177 and clarify

773d0dd

row IDs

572cdf1

alexcjohnson requested review from Marc-Andre-Rivet and chriddyp April 21, 2019 23:10

chriddyp temporarily deployed to dash-table-review-pr-412 April 21, 2019 23:11 Inactive

changelog for row IDs

edd86e9

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 00:08 Inactive

fix doubleclick with row IDs

6bb24ad

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 00:22 Inactive

for realz: row/col IDs can be string or number!

c48ea44

I was a bit schizophrenic about this before...

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 01:04 Inactive

update percy storybook tests for row IDs

b1f2a39

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 01:30 Inactive

alexcjohnson added 2 commits April 22, 2019 00:30

fix next-page test - now paging clears cell selection

e203497

lint percy tests: no global data/columns, so we can use these as args

76b59a9

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 04:43 Inactive

alexcjohnson commented Apr 22, 2019

View reviewed changes

more robust handling of missing active_cell

35dc91d

chriddyp temporarily deployed to dash-table-review-pr-412 April 22, 2019 06:43 Inactive

Marc-Andre-Rivet reviewed Apr 23, 2019

View reviewed changes

alexcjohnson added 2 commits April 23, 2019 12:41

🔪 * as R from paginator

45928b0

prevent blank page at the end of pagination

8be7509

Marc-Andre-Rivet reviewed Apr 23, 2019

View reviewed changes

alexcjohnson added 3 commits April 23, 2019 16:38

propTypes can't reference patterns

397b914

stricter python linting

8f01c25

clean up cell click/shift-click logic

1c69cfb

chriddyp temporarily deployed to dash-table-review-pr-412 April 23, 2019 21:57 Inactive

fix cell click logic for clicking the active cell again

3e31a7f

chriddyp temporarily deployed to dash-table-review-pr-412 April 23, 2019 23:15 Inactive

alexcjohnson added 2 commits April 24, 2019 18:49

clear selection on filter or sort

7738a3a

expand select_(row->props)_test to cover new rowID & related props

1490265

chriddyp temporarily deployed to dash-table-review-pr-412 April 24, 2019 22:52 Inactive

lint

196416f

chriddyp temporarily deployed to dash-table-review-pr-412 April 25, 2019 00:00 Inactive

alexcjohnson mentioned this pull request Apr 25, 2019

change sorting_settings to sort_by #417

Merged

Merge branch 'master' into row-ids

b6a5f55

chriddyp temporarily deployed to dash-table-review-pr-412 April 25, 2019 15:26 Inactive

Merge branch 'master' into row-ids

2ed6728

chriddyp temporarily deployed to dash-table-review-pr-412 April 26, 2019 14:04 Inactive

Marc-Andre-Rivet reviewed Apr 26, 2019

View reviewed changes

lint for updated TS

5cec33f

chriddyp temporarily deployed to dash-table-review-pr-412 April 26, 2019 17:03 Inactive

warn on switchCell with no active_cell

9420590

chriddyp deployed to dash-table-review-pr-412 April 26, 2019 19:32 View deployment

Marc-Andre-Rivet approved these changes Apr 26, 2019

View reviewed changes

alexcjohnson merged commit 6aa16df into master Apr 26, 2019

alexcjohnson deleted the row-ids branch April 26, 2019 19:56

Uh oh!

Conversation

alexcjohnson commented Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriddyp commented Apr 22, 2019

Uh oh!

alexcjohnson Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexcjohnson commented Apr 21, 2019 •

edited

Loading

alexcjohnson Apr 22, 2019 •

edited

Loading

Marc-Andre-Rivet Apr 23, 2019 •

edited

Loading