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

MRT_ShowHideColumnsMenu showAll / hideAll is poorly implemented #1277

Open
1 task done
bigabig opened this issue Oct 23, 2024 · 2 comments · May be fixed by #1336
Open
1 task done

MRT_ShowHideColumnsMenu showAll / hideAll is poorly implemented #1277

bigabig opened this issue Oct 23, 2024 · 2 comments · May be fixed by #1336

Comments

@bigabig
Copy link

bigabig commented Oct 23, 2024

material-react-table version

3.0.1

react & react-dom versions

18.3.1

Describe the bug and the steps to reproduce it

Hi,
I noticed that clicking the showAll or hideAll button of the MRT_ShowHideColumnsMenu is not optimized.

If I have 30 columns, it will call the corresponding onRowSelectionChange 30 times. This should be optimized to be only one single call, updating the state only once.

Actually, this is not a problem, when you are using React's useState and provide the setState function. However, with the current implementation, I encounter problems when trying to synchronize with global state (in my case redux), as this will dispatch 30 redux actions (instead of just one).

I have not checked the source code of MRT_ShowHideColumnsMenu, but I think this should be fairly easy to fix / improve?
If you do not intend to address this, can you guide me on how to implement MRT_ShowHideColumsnMenu myself?

Thank you!

Minimal, Reproducible Example - (Optional, but Recommended)

const table = useMaterialReactTable({
    data: searchSpace,
    columns: columns,
    state: {
      rowSelection: rowSelectionModel,
    },
    onRowSelectionChange: setRowSelectionModel,
});

with useState this is fine, e.g.
const [rowSelectionModel, setRowSelectionModel] = useState<T>(reduxState);

with redux this is not fine, e.g.

const rowSelectionModel = useAppSelector(reduxStateSelectorFn);
const setRowSelectionModel = useCallback(
  (updater: MRT_Updater<T>) => {
      const newState = updater instanceof Function ? updater(rowSelectionModel) : updater;
      dispatch(reduxAction(newState));
    }
  },
  [dispatch, reduxAction],
);

Screenshots or Videos (Optional)

No response

Do you intend to try to help solve this bug with your own PR?

None

Terms

  • I understand that if my bug cannot be reliably reproduced in a debuggable environment, it will probably not be fixed and this issue may even be closed.
@bigabig
Copy link
Author

bigabig commented Oct 23, 2024

the reason why it is not fine with redux is that the rowSelectionModel only changes after re-render. As a consequence, only the last of the 30 state updates comes through.

@dangkhoa99
Copy link
Contributor

@bigabig , You can try this way:

  React.useEffect(() => {
    dispatch(
      setReduxAction({
        key: 'rowSelection',
        value: table.getState().rowSelection,
      }),
    );
  }, [table.getState().rowSelection]);

fredericbahr added a commit to fredericbahr/material-react-table that referenced this issue Dec 21, 2024
The functionality to show/hide all columns within the ShowHideColumnsMenu was implemented with a custom function which filters all columns and toggles the visibility of each column. This calls the `onColumnVisibilityChange` callback multiple times.

In my opinion this can be refactored to use the tables `toggleAllColumnsVisible` API, which results in only one call of `onColumnVisibilityChange` and makes the code easier to understand.

Fixes: KevinVandy#1277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants