Docstrings #173

chriddyp · 2018-10-24T20:46:17Z

No description provided.

chriddyp · 2018-10-24T20:46:48Z

Fixes #163

chriddyp · 2018-10-24T20:50:25Z

These prop-types will end up in the documentation on dash.plot.ly.

There are a few TODOs in there - @Marc-Andre-Rivet can you clarify them for me?

I've linked many of the comments to GitHub issues as a way to better warn folks about properties that will probably change when we go from alpha to beta.

Check it out @Marc-Andre-Rivet @valentijnnieman @cldougl

Marc-Andre-Rivet · 2018-10-24T20:57:27Z

src/dash-table/Table.js

+
+        /**
+         * TODO - What does this do?
+         */


This is used by the dropdown to know whether the value can be deleted (the 'x' is visible on hover)

Marc-Andre-Rivet · 2018-10-24T21:02:01Z

src/dash-table/Table.js

+         * If True, then the data associated with this column is editable.
+         * Note that this can be toggled on a per-table basis with the
+         * higher level `editable` flag.
+         */


Note that the table as a whole must be editable for this field to apply. A non-editable table + an editable field still results in a readonly cell. We could modify the table beahvior so that if table.editable is False, column.editable True overrides and vice versa. Right now it's inconvenient to have a table with n columns and only a small subset of editable columns.

Marc-Andre-Rivet · 2018-10-24T21:02:34Z

src/dash-table/Table.js

+
+        /**
+         * If True, then the name of this column is editable.
+         */


Behavior with number (multi-lined header) missing

Marc-Andre-Rivet · 2018-10-24T21:05:16Z

src/dash-table/Table.js

+     * two common behaviors:
+     * TODO - `fit` - The table will ...?
+     * `grow` - The table will expand to its parents container.
+     */


fit -> the table container will fit the content and not try to take all the available space in the parent
grow -> the table will try and aggressively take all available space

Similar idea is greedy vs. non-greedy regex.

This is clearly a prop that we will want to rework and/or get rid of if we can.

Marc-Andre-Rivet · 2018-10-24T21:05:46Z

src/dash-table/Table.js

+     * If True, then the data in all of the cells is editable.
+     * Note that this can be toggled on a per-column basis with the
+     * `editable` flag inside the `columns` property.
+     */


See column.editable comment above

Marc-Andre-Rivet · 2018-10-24T21:06:31Z

src/dash-table/Table.js

+     * so `n_fixed_columns=1` will fix the header row,
+     * `n_fixed_rows=2` will fix the header row and the first row,
+     * or the first two header rows (if there are multiple headers).
+     */


Same note as for n_fixed_columns applies here.

Marc-Andre-Rivet · 2018-10-24T21:07:29Z

src/dash-table/Table.js

+     * `selected_cell` represents the cell that is currently
+     * selected.
+     * TODO - How is this different than `active_cell`?
+     */


selected_cell contains all the cells that are currently selected in the table while active_cell is the one with "focus"

Shouldn't it be selected_cells then?

Marc-Andre-Rivet · 2018-10-24T21:08:26Z

src/dash-table/Table.js

+     * - `'fe'` refers to "front-end" paging: passing large data up-front
+     * - `'be'` refers to "back-end" paging: updating the data on the fly via callbacks
+     * - `False` will disable paging, attempting to render all of the data at once
+     * TODO - What does `True` do?


True is the same as 'fe'

Marc-Andre-Rivet · 2018-10-24T21:10:03Z

src/dash-table/Table.js

+     * - `current_page` represents which page the user is on.
+     * Use this property to index through data in your callbacks with
+     * backend paging.
+     * TODO - What does `displayed_pages` to?


I think displayed_pages can be removed, original implementation was taking page and infinite scroll into consideration. It should be removed.

Marc-Andre-Rivet · 2018-10-24T21:10:58Z

src/dash-table/Table.js

+
+    /**
+     * TODO - What does `navigation` do?
+     */


navigation should be PropTypes.oneOf(['page']) and would include other strategies like 'infinite_scroll' or similar in the future.

It could be removed entirely at the moment.

Marc-Andre-Rivet · 2018-10-24T21:11:58Z

src/dash-table/Table.js

+     * responsibility of the developer to program the filtering
+     * through a callback (where `filtering_settings` would be the input
+     * and `data` would be the output).
+     * TODO - What does `True` do?


True behaves the same as 'fe'

Marc-Andre-Rivet · 2018-10-24T21:12:49Z

src/dash-table/Table.js

+     * with the `fe` setting or via a callback in your
+     * python "back-end" with the `be` setting.
+     * Clicking on the sort arrows will update the
+     * `sorting_settings` property.


True is the same as 'fe'

Marc-Andre-Rivet · 2018-10-24T21:15:06Z

src/dash-table/Table.js

+    /**
+     * TODO - Is this the `selected_rows` or just the indicies w.r.t.
+     * the original data?
+     */


This is not the selected_rows. This array indicates the order in which the original data rows appear after being filtered and sorted.

Marc-Andre-Rivet · 2018-10-24T21:15:43Z

src/dash-table/Table.js

+    /**
+     * TODO - Is this the `selected_rows` or just the indicies w.r.t.
+     * the original data?
+     */


This is not the selected_rows. This array indicates the order in which the original data rows appear after being filtered and sorted and paged. While virtual encompasses the entire original data, this is limited to the items currently visible to the user.

Marc-Andre-Rivet · 2018-10-24T21:17:14Z

src/dash-table/Table.js

    derived_virtual_indices: PropTypes.arrayOf(PropTypes.number),

+    /**
+     * DEPRECATED
+     */


While column.options is fully deprecated (it can be replaced without loss with column_static_dropdown), derived_properties still offers a unique way of mapping dropdown values by datum/row index.

valentijnnieman

Couple of nitpicks, using the new Github suggestion feature here and there, feel free to improve them or ignore them. Looks good!

src/dash-table/Table.js

valentijnnieman · 2018-10-24T22:59:28Z

src/dash-table/Table.js

    css: PropTypes.arrayOf(PropTypes.shape({
        selector: PropTypes.string.isRequired,
        rule: PropTypes.string.isRequired
    })),

+    /**
+     * The contents of the table.
+     * The keys in item in data should match the column IDs.


"The keys in item in data..." not sure what you mean here, a typo maybe?

valentijnnieman · 2018-10-24T23:03:18Z

src/dash-table/Table.js

+     * `selected_cell` represents the cell that is currently
+     * selected.
+     * TODO - How is this different than `active_cell`?
+     */


Shouldn't it be selected_cells then?

src/dash-table/Table.js

valentijnnieman · 2018-10-24T23:12:27Z

src/dash-table/Table.js

+    /**
+     * When we sort, should empty strings (`''`) be considered
+     * valid values (they will appear first when sorting ascending)
+     * or should they be ignored (always appearing last)?


Not sure if this is a question for us (missing a TODO?) or if it's the actual docstring, haha. If it is the docstring, maybe it's better to rewrite so it doesn't ask a question.

chriddyp · 2018-10-25T00:53:24Z

Thanks @Marc-Andre-Rivet and @valentijnnieman ! I just incorporated your edits.

Marc-Andre-Rivet

lgtm

Many of these props will change in the future, I’ve linked properties to the following issues: - #166 - #167 - #168 - #168 - #167 - #169 - #169 - #169 - #170 - #171

Co-Authored-By: chriddyp <chris@plot.ly>

chriddyp temporarily deployed to dash-table-review-pr-173 October 24, 2018 20:46 Inactive

chriddyp requested review from Marc-Andre-Rivet, valentijnnieman and cldougl October 24, 2018 20:46

Marc-Andre-Rivet reviewed Oct 24, 2018

View reviewed changes

valentijnnieman reviewed Oct 24, 2018

View reviewed changes

chriddyp temporarily deployed to dash-table-review-pr-173 October 25, 2018 00:01 Inactive

chriddyp temporarily deployed to dash-table-review-pr-173 October 25, 2018 00:02 Inactive

chriddyp temporarily deployed to dash-table-review-pr-173 October 25, 2018 00:53 Inactive

Marc-Andre-Rivet approved these changes Oct 25, 2018

View reviewed changes

chriddyp and others added 9 commits October 25, 2018 11:30

📝 prop type comments

c50adb7

Many of these props will change in the future, I’ve linked properties to the following issues: - #166 - #167 - #168 - #168 - #167 - #169 - #169 - #169 - #170 - #171

📦 new build

47da6cf

📦 rc9

729da95

📝

6347a77

Co-Authored-By: chriddyp <chris@plot.ly>

📝

c6d931e

Co-Authored-By: chriddyp <chris@plot.ly>

📝

b1d5b8d

Co-Authored-By: chriddyp <chris@plot.ly>

📝

52d7c82

Co-Authored-By: chriddyp <chris@plot.ly>

📝

4a5b431

Co-Authored-By: chriddyp <chris@plot.ly>

📝 update copy

1e2e2de

chriddyp force-pushed the docstrings branch from d4aec0d to 1e2e2de Compare October 25, 2018 15:54

chriddyp deployed to dash-table-review-pr-173 October 25, 2018 15:54 View deployment

chriddyp merged commit e38721d into master Oct 25, 2018

chriddyp deleted the docstrings branch October 25, 2018 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docstrings #173

Docstrings #173

chriddyp commented Oct 24, 2018

chriddyp commented Oct 24, 2018

chriddyp commented Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

valentijnnieman Oct 24, 2018

chriddyp Oct 25, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

Marc-Andre-Rivet Oct 24, 2018

valentijnnieman left a comment

valentijnnieman Oct 24, 2018

valentijnnieman Oct 24, 2018

valentijnnieman Oct 24, 2018

chriddyp commented Oct 25, 2018

Marc-Andre-Rivet left a comment

Docstrings #173

Docstrings #173

Conversation

chriddyp commented Oct 24, 2018

chriddyp commented Oct 24, 2018

chriddyp commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentijnnieman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriddyp commented Oct 25, 2018

Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment