Add loading states and prevent cells from being edited when table is loading. by shammamah-zz · Pull Request #484 · plotly/dash-table

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

shammamah-zz · 2019-06-28T20:47:22Z

Closes#319.

About

Added loading state to Table props
Disabled editing and copy-paste when data property of Table is loading
Added tests

Marc-Andre-Rivet

Few comments below for the handling of onChange and cell content.

Marc-Andre-Rivet · 2019-08-29T14:16:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


This approach is good to handle doubleClick but will cause unexpected behavior with onChange.
As implemented, a user could do the following steps:
select a cell and edit it, triggering a callback
while the callback executes, a new cell is selected for editing
the user enters a new value and submits it
the value resets to the original value in the cell
This behavior would be surprising. Better to prevent the user from editing the cell content altogether.
The table cells are generated in two steps, one step that does not take user interactions into consideration and one that does:
a "partial" generation that does not take into account the active cells, (loading_state), etc. (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L67)
a "final" generation that edits the result of step 1 (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L92)
I think this "final" generation step is where we would want to take the loading_state into consideration.
The getContent method below would need to be able to take loading_state into consideration for this case, to override column.editable (https://github.com/plotly/dash-table/blob/master/src/dash-table/derived/cell/contents.tsx#L128)

Changes made in 60c952f and fc77aec.

(This should also cover the copy/paste use case, if I understand correctly)

Marc-Andre-Rivet · 2019-08-29T14:41:50Z

 const realIdx = virtualized.indices[idx];

- if (!c.editable) {
+ if (!c.editable || loading_state && loading_state.is_loading) {


loading_state && loading_state.is_loading
Listening for all loading_states works but I wonder if we could limit ourselves to a subset of all props. Is there anything besides data that really impacts editing?

Playing with this, I realized there was a bug for loading_state and multi output: plotly/dash#893

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet

Waiting for 🌴 for evaluated loading_state.
This will require a follow up PR in dash-docs, at least to explain the table behavior and what triggers it.
Add changelog entry

…ntrolledTable props.

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Marc-Andre-Rivet · 2019-09-12T15:35:09Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


loading_state is not required for partial resolution of the cells as they will all be considered inactive anyway. This can be removed and partialGet can pass false to getContent instead.

Marc-Andre-Rivet · 2019-09-12T15:35:57Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ loading_state


Calculate the boolean value prior to passing it to cellContents. On each render the object will potentially be different and force re-evaluation / bust cache very often otherwise.

Marc-Andre-Rivet · 2019-09-12T16:01:54Z

 !!is_focused,
- dropdowns
+ dropdowns,
+ data_loading


Better, but what I meant is that we can omit this parameter completely for partialGet at the moment.

Right, that makes sense!

Compute relevant information (whether the data are loading) in CellFactory instead.

See: #484 (comment)

Marc-Andre-Rivet · 2019-09-12T16:08:01Z

 return (!dropdown || !editable) ? CellType.DropdownLabel : CellType.Dropdown;
 default:
- return (!active || !editable) ? CellType.Label : CellType.Input;
+ return (!active || !editable || is_loading) ? CellType.Label : CellType.Input;


As is, won't the dropdowns still be editable when the table is loading?

Making it a label has UI/presentation implications though.. I don't remember if the dropdown library we use allows us to make the dropdown readonly

That's why I left the loading states info out of the dropdowns -- wasn't sure exactly what would happen or what "editing" means in that case.

Looking at the documentation for react-select there seems to be a isDisabled prop
https://react-select.com/props
It reacts somewhat like we would want I think -- except maybe for default styling
https://jedwatson.github.io/react-select/

Marc-Andre-Rivet

💃 Looks good to me

chriddyp temporarily deployed to dash-table-review-pr-484 June 28, 2019 20:47 Inactive

shammamah-zz force-pushed the loading-states-uneditable branch from 39a98c4 to 170adb2Compare June 28, 2019 20:50

chriddyp temporarily deployed to dash-table-review-pr-484 June 28, 2019 20:51 Inactive

chriddyp deployed to dash-table-review-pr-484 July 2, 2019 15:29 View deployment

shammamah-zz force-pushed the loading-states-uneditable branch from e027042 to e1cb857Compare July 24, 2019 17:44

shammamah-zz force-pushed the loading-states-uneditable branch from ecaf7d2 to f866ceeCompare August 26, 2019 18:00

Marc-Andre-Rivet suggested changes Aug 29, 2019
View reviewed changes

Marc-Andre-Rivet mentioned this pull request Aug 29, 2019
CSS Loading States don't work with multi-output // Empty component_name and prop_name on loading_state for callbacks with multiple output plotly/dash#893
Open

shammamah-zz force-pushed the loading-states-uneditable branch from 36390b3 to e521ae9Compare September 6, 2019 20:18

shammamah-zz mentioned this pull request Sep 6, 2019
Cypress server copy/paste test failing on certain machines #577
Closed

Shammamah Hossain added 2 commits September 9, 2019 11:48

Add loading state to DataTable props; add loading state interface.
1bb06ab

Use loading state to compute cell type (label or input).
41a3f2a

shammamah-zz force-pushed the loading-states-uneditable branch from 865885a to 0443f25Compare September 9, 2019 15:50

shammamah-zz marked this pull request as ready for review September 9, 2019 15:52

shammamah-zz requested a review from alexcjohnson as a code owner September 9, 2019 15:52

shammamah-zz requested a review from Marc-Andre-Rivet September 9, 2019 15:52

shammamah-zz force-pushed the loading-states-uneditable branch from 0443f25 to 1ec4b98Compare September 9, 2019 16:19

Marc-Andre-Rivet reviewed Sep 10, 2019
View reviewed changes

Comment threadsrc/dash-table/components/CellFactory.tsx Outdated

shammamah-zz force-pushed the loading-states-uneditable branch from 7a29749 to 5b88a69Compare September 10, 2019 16:09

Shammamah Hossainand others added 4 commits September 10, 2019 12:26

Update copy-paste server test with cy.wait.
051954e

Add tests for uneditable loading states.
e029f1b

Update src/dash-table/components/CellFactory.tsx
2a48aa0
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Prevent pasting when table data are loading.
2da14e0

shammamah-zz force-pushed the loading-states-uneditable branch 3 times, most recently from b712269 to 955e040Compare September 11, 2019 19:43

shammamah-zz requested a review from Marc-Andre-Rivet September 11, 2019 20:13

Marc-Andre-Rivet suggested changes Sep 11, 2019
View reviewed changes

Shammamah Hossain added 2 commits September 11, 2019 17:15

Wait for inputs to appear before pasting.
f81a3dd

Move computation of data loading to separate file, and remove from Co…
8dd3fe6
…ntrolledTable props.

shammamah-zz force-pushed the loading-states-uneditable branch from 5822a41 to 8dd3fe6Compare September 11, 2019 21:15

Add entry to CHANGELOG.
8d67c79

Marc-Andre-Rivet mentioned this pull request Sep 12, 2019
Table loading state - Fix tests #581
Merged

Shammamah Hossainand others added 4 commits September 12, 2019 10:27

Add state and CSS selector for editable cells.
80966c4
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Update Cypress tests.
8d36fdb
Read new State prop from ControlledTable; add timeouts of 1000ms before editing in copy_paste_test. Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Fix Python apps used in server test.
1c1f2c8
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

Add default test for delete_cells_test.
b465203
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>

shammamah-zz force-pushed the loading-states-uneditable branch from c99ff6b to b465203Compare September 12, 2019 14:36

shammamah-zz requested a review from Marc-Andre-Rivet September 12, 2019 14:54

Marc-Andre-Rivet reviewed Sep 12, 2019
View reviewed changes

Shammamah Hossain added 2 commits September 12, 2019 12:02

Move loading state out of calls to cellContents and partialGet.
042a1b0
Compute relevant information (whether the data are loading) in CellFactory instead.

Always pass in false for data_loading in partialGet.
40722c1
See: #484 (comment)

shammamah-zz force-pushed the loading-states-uneditable branch from 42c7a48 to 40722c1Compare September 12, 2019 16:06

Marc-Andre-Rivet reviewed Sep 12, 2019
View reviewed changes

shammamah-zz requested a review from Marc-Andre-Rivet September 12, 2019 16:34

Disable dropdowns when data are loading.
2d15ca4

shammamah-zz force-pushed the loading-states-uneditable branch from f02acb8 to 2d15ca4Compare September 12, 2019 17:59

Shammamah Hossain added 2 commits September 12, 2019 15:18

Add visual tests for disabled dropdowns.
b8fd905

Add unit test for data_loading.
c77b2cd

Marc-Andre-Rivet approved these changes Sep 12, 2019
View reviewed changes

shammamah-zz merged commit f9b7bc4 into masterSep 12, 2019

shammamah-zz deleted the loading-states-uneditable branch September 12, 2019 19:59