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

Resizing columns fails while the UI is updating. #268

Closed
Jamjarman opened this issue Dec 5, 2017 · 9 comments
Closed

Resizing columns fails while the UI is updating. #268

Jamjarman opened this issue Dec 5, 2017 · 9 comments

Comments

@Jamjarman
Copy link
Contributor

Jamjarman commented Dec 5, 2017

While the UI is updating either because row data is updating or new rows are being added resizing columns fails. The onColumnResizeEndCallback function gets undefined as a column key instead of the actual columnKey.

Expected Behavior

Column resizing should work while the UI is updating (Or it should be disabled entirely while the UI updates).

Current Behavior

Currently the user can drag the columns and the bar appears to indicate the new size, however once the user releases the click nothing happens.

Possible Solution

I believe that this is the same issue reference here on the original fixed data table, and again on fixed-data-table-2: #139.

It seems like the resizing is somehow tied to the UI state although I haven't done enough poking to know exactly what's going on. It's not entirely clear to me what the purpose of isColumnResizing is as well.

Steps to Reproduce (for bugs)

Checkout this jsfiddle. Try and resize a column. A message next to the "Start Data Load" button should read "Column # updated with width ###".

Now hit the start loading data and then try and resize a column immediately. You should see a message next to the button saying "Column undefined updated with width ###"

Context

Our use case for this grid includes displaying information on a number of tasks as they execute in real time. This requires us to constantly update the grid as the data in each cell changes. What this means is that while our jobs are executing it is impossible to resize a grid.

Your Environment

  • Version used:
    0.8.6
  • Browser Name and version:
    Chrome 55.0.2883.87, 62.0.3202.75; Firefox 50.1.0;
  • Operating System and version (desktop or mobile):
    Windows 10
@wcjordan
Copy link
Member

wcjordan commented Dec 6, 2017

Thanks for a solid issue report.

I believe this is happening because you've hardcoded isColumnResizing={false} in your table props. Instead replace that with a value which you omit when updating during loads but include when updating after onColumnResizeEnd.

This is an artifact of an unfortunate bad API decision that splits management of the state between the component & the outside props...

I'm going to close, but feel free to follow up with questions and I'll do what I can to help.

Also if you want to look at fixing this internal to the component by adding a wrapper around onColumnResizeEndCallback in FixedDataTable.js, please do (see isColumnReordering for reference). We should be careful not to break the API though.

@Jamjarman
Copy link
Contributor Author

Hi,

I'm not sure what you mean by

Instead replace that with a value which you omit when updating during loads but include when updating after onColumnResizeEnd.

What value should be included when updating after onColumnResizeEnd, and by "a value which you omit" do you mean that the property shouldn't be set at all?

Thanks much!

@wcjordan
Copy link
Member

wcjordan commented Dec 7, 2017

Yep you got it. Omit it when you want FDT to continue to control it. Set as isColumnResizing={false} on the update following onColumnResizeEnd.

Imagine FDT has functionality to start column resizing built in, but it never stops resizing unless told to. Instead it tells you when someone released the mouse and expects you to tell it to stop. And instead of giving you a method to call to tell it to stop, it wants you to render it with a prop telling it that. But if you leave that prop in place it will think you want it to stop resizing on all updates...

Definitely not a very intuitive API...

@Jamjarman
Copy link
Contributor Author

If I'm understanding you correctly something like this fiddle should solve the problem however it seems to persist.

Here I've added a state for isColumnResizing which starts as null and is set to false in onColumnResizeEnd.

Greatly appreciate your guidance on this!

@wcjordan
Copy link
Member

wcjordan commented Dec 7, 2017

You'll also want to set isColumnResizing back to undefined after the render when it's false. Also it needs to be totally omitted since React passes undefined props through to the component & FDT uses object spread to override values (i.e. undefined will override true if isColumnResizingProps=undefined).

I've tweeked your example to fix the issue here:
https://jsfiddle.net/L60ae8bz/1/

I did so by adding at the top of render

    const isColumnResizingProps = {};
    if (this.state.isColumnResizing === false) {
      setTimeout(() => {
        this.setState({
          isColumnResizing: undefined,
        });
      });
      isColumnResizingProps.isColumnResizing = false
    }

and changing the render to have

<Table
  ...
  {...isColumnResizingProps}>
...

I also had to change handleButtonClick to not alter isColumnResizing, and onColumnResizeEndCallback to set it to false.

@Jamjarman
Copy link
Contributor Author

I think I understand now, however it looks like in that jsfiddle the problem still persists. When I resize columns while updating data the columnKey is returned as undefined.

We've been updating column size by keeping a mapping of key to size which we update in the onColumnResizeEnd method which won't work if they key is undefined. Is this still the expected behavior and if so is there another way to do the resizing?

@wcjordan
Copy link
Member

wcjordan commented Dec 7, 2017

You're right, that data's getting lost. Here's a patch which I believe will fix if you want to test and put up a PR if it works:

In FixedDataTable.js inside _calculateState on line 1176

-    if (props.isColumnResizing) {
+    if (props.isColumnResizing || (oldState && oldState.isColumnResizing)) {

@wcjordan wcjordan reopened this Dec 7, 2017
Jamjarman added a commit to Jamjarman/fixed-data-table-2 that referenced this issue Dec 8, 2017
Check the old state while resizing so that state isn't lost if resizing occurs while the data is being updated. schrodinger#268
wcjordan pushed a commit that referenced this issue Dec 9, 2017
Check the old state while resizing so that state isn't lost if resizing occurs while the data is being updated. #268
@wcjordan
Copy link
Member

Released w/ v0.8.8

@wcjordan
Copy link
Member

wcjordan commented Jan 9, 2018

Heads up I had to tweak this w/ v0.8.9, but it shouldn't break anything

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

No branches or pull requests

2 participants