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

add setSelection to tableControl + add onOrderByChange and onSelectionChange to Hightable #21

Closed
wants to merge 1 commit into from

Conversation

severo
Copy link
Contributor

@severo severo commented Jan 8, 2025

Note that I moved the reducer function inside the Hightable component, in order to use the new props (onOrderByChange and onSelectionChange). Seen in https://overreacted.io/a-complete-guide-to-useeffect ("Why useReducer Is the Cheat Mode of Hooks"). Note sure if it's a good practice to do so.

Missing: tests

@severo
Copy link
Contributor Author

severo commented Jan 9, 2025

Note that it goes against this principle in redux (not react):

https://redux.js.org/style-guide/#reducers-must-not-have-side-effects

Seen here: https://stackoverflow.com/questions/66420286/accessing-relying-on-props-inside-usereducer-an-anti-pattern

I'm not sure if we want to use this.

Two alternatives:

  1. use a shared state (redux) between the app and the component
  2. lift the state up to the parent component

But the issue is that we distribute the component as an independent package, so: 1. would not work (and I don't think we need an external store lib), and 2. would mean that we pass orderBy and setOrderBy to HighTable and manage no local state (same for rows selection), ie. using the sort and the selection would require the parent to keep the state. It could make sense, but it would mean that if you don't provide these props, you don't have the sort and selection features.

To be honest, I would prefer alternative 2 to the current implementation, which redefines the reducer each time the order or selection changes.

I'll prepare another PR with the code.

WDYT @platypii?

After trying a bit, I think the HighTable component has to handle a local state itself, as the state is intricate and updating the order has side effects on the rows and the selection for example. Let's try with the "useEffect hack", I think.

@severo
Copy link
Contributor Author

severo commented Jan 9, 2025

I think the solution in #22 is much better: you choose to let the component manage its state (HighTable) or you take control of it in the parent (ControlledHighTable)

@severo severo force-pushed the control-order-and-selection-from-outside branch from 6025e59 to a200d8a Compare January 16, 2025 12:37
@severo
Copy link
Contributor Author

severo commented Jan 22, 2025

Closing in favor of #22

@severo severo closed this Jan 22, 2025
@severo severo deleted the control-order-and-selection-from-outside branch January 22, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant