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

Control the selection and sort #22

Merged
merged 31 commits into from
Jan 22, 2025
Merged

Control the selection and sort #22

merged 31 commits into from
Jan 22, 2025

Conversation

severo
Copy link
Contributor

@severo severo commented Jan 9, 2025

Add four props to control the selection and the sort: selection, onSelectionChange, orderBy and onOrderByChange.

See how we use it in an app: https://github.com/hyparam/hyperparam-cli/pull/139/files

Replaces #21, since having a reducer depending on props seems to be a bad practice.

Note that it removes the need for tableControl (I removed it).

@severo
Copy link
Contributor Author

severo commented Jan 10, 2025

Update:

I removed selection and anchor from the internal state in ControlledHighTable and replaced them with two props: selectionAndAnchor and setSelectionAndAnchor.

In HighTable, I handle the selection and anchor in the reduced state, but I added an optional callback onSelectionChange to provide the value to the parent (while not providing a way to set the selection).


edit: oops, I broke the tests, I'll update on Monday

Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's generally heading in the right direction with SelectionAndAnchor stuff.

I'm a little suspicious about exposing the state and dispatch as props which are meant to be InternalActions but actually leak out.

Specifically orderBy needs to be settable from outside the component, and I see in the demo that you do a dispatch with an "internal" SET_ORDER message. This works but feels weird. I think ideally there would be no state and dispatch exposed as props, and instead have orderBy and setOrderBy managed by the parent?

Alternatively there could be an ExternalState type with both selectionAndAnchor and also orderBy?

I feel like the ideal world is that InternalState is managed by an internal reducer that doesn't need to be exposed. We hoist only the state and setters needed as props to expose to the parent.

The thing I am worried about is losing the benefits of a reducer of doing predictable atomic state updates. For example: if we need to clear the selection when orderBy changes, does the parent component need to sync two useStates? Or is there an InternalReducer in HighTable and also be an ExternalReducer in the parent component which receives ExternalAction and updates the ExternalState accordingly?

@severo
Copy link
Contributor Author

severo commented Jan 11, 2025

Re orderBy: indeed, I only did the change for selection, but not for order yet, so the current state is a weird mix.

But I also agree that the reducer is useful when a change in one property has consequences on the other ones (order by resets the selection for example). And I'm not sure yet how to handle it best.

@severo severo force-pushed the pass-reducer-to-component branch from e6218c2 to f177d37 Compare January 16, 2025 13:36
I try to follow the same logic as
https://react.dev/reference/react-dom/components/input and
https://react.dev/learn/sharing-state-between-components#controlled-and-uncontrolled-components

The selection in HighTable is controlled, or uncontrolled, and it cannot
be changed once the component has been created.

Four modes:
   * - controlled (selection and onSelectionChange are defined): the parent controls the selection and receives the user interactions. No local state.
   * - controlled read-only (selection is defined, onSelectionChange is undefined): the parent controls the selection and the user interactions are disabled. No local state.
   * - uncontrolled (selection is undefined, onSelectionChange is defined): the component controls the selection and the user interactions. Local state.
   * - disabled (selection and onSelectionChange are undefined): the selection is hidden and the user interactions are disabled. No local state.

Next steps:
- do the same for orderBy
- merge ControlledHighTable back to HighTable
- fix tests
- handle selection with keyboard (onChange on checkboxes)
@severo severo force-pushed the pass-reducer-to-component branch from d30143d to db88dc9 Compare January 17, 2025 14:31
@severo severo force-pushed the pass-reducer-to-component branch from c8cfcce to 6aea297 Compare January 20, 2025 10:32
@severo severo changed the title Publish two components: HighTable and ControlledHighTable Control the selection and sort Jan 20, 2025
@severo severo marked this pull request as draft January 20, 2025 22:55
@severo severo marked this pull request as ready for review January 21, 2025 13:27
@severo severo requested a review from platypii January 21, 2025 13:28
Copy link
Contributor

@platypii platypii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid refactoring, with much more clear, typesafe, and flexible API! Using hooks does seem like the right choice here. Thanks for fighting through this I'm sure this was not easy to get right.

README.md Show resolved Hide resolved
src/sort.ts Outdated Show resolved Hide resolved
test/HighTable.test.tsx Outdated Show resolved Hide resolved
test/HighTable.test.tsx Outdated Show resolved Hide resolved
@severo severo merged commit e9a81d3 into master Jan 22, 2025
8 checks passed
@severo severo deleted the pass-reducer-to-component branch January 22, 2025 09:59
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.

2 participants