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

[DataGrid] Prepare work for a future public state api #533

Merged
merged 11 commits into from Nov 11, 2020

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Nov 3, 2020

Add the state API that allows using the grid in a controlled way. Related to #190.

@dtassone dtassone marked this pull request as draft November 3, 2020 14:46
@dtassone dtassone marked this pull request as ready for review November 4, 2020 13:47
Copy link
Member

@oliviertassinari oliviertassinari 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 that we miss:

  • The declarative API, this one should come first.
  • The documentation.
  • End to end tests.
  • A description of the problem we are solving.

@dtassone
Copy link
Member Author

dtassone commented Nov 4, 2020

I think that we miss:

  • The declarative API, this one should come first.
    What do you mean?
  • The documentation.
  • End to end tests.
  • A description of the problem we are solving.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 4, 2020

What do you mean?

  • Looking closer, it might be handled, I think that I will wait to see the documentation and the tests to be sure.
  • I assume we miss a prop to set the default state, should it be handled in a different pull request, later on?
  • On a related, the controllable API of react-table is puzzling me. I have never seen such a pattern before. I don't understand the incentive for having a prop that expects a state function (oldState => newState). Strange. It seems to have been introduced in Override internal table state (beta 26) TanStack/table#1704. I don't see any completing argument in the thread. So, I think that we can continue in the direction we have started, and ignore it.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

For the documentation, I think that a new page would probably be the best solution. I don't see another place where documenting this really makes sense. We could maybe have a "Data & state" page, where we document the state, how to load data, lazy loading, the server-side part,

packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
@dtassone dtassone force-pushed the stateApi branch 2 times, most recently from 1c818b0 to 57caf63 Compare November 6, 2020 12:24
@oliviertassinari oliviertassinari changed the title [DataGrid] State api [DataGrid] Introduce public state api Nov 6, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

no 📚 = nobody uses it

packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Agree with Danail, exposing the state to the open raises the question of what's internal and not. Let's have all new features be documented. This is a forcing function to consider the feature end-to-end.

@dtassone
Copy link
Member Author

Agree with Danail, exposing the state to the open raises the question of what's internal and not. Let's have all new features be documented. This is a forcing function to consider the feature end-to-end.

This is to allow to persist the state, attach it to a redux store, or just manipulate the grid state directly.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 10, 2020

This is to allow to persist the state, attach it to a redux store, or just manipulate the grid state directly.

Will it work? I am right to assume that we can't have developers store internal values that we might change between two release patches, breaking the restore logic?

@dtassone
Copy link
Member Author

This is to allow to persist the state, attach it to a redux store, or just manipulate the grid state directly.

Will it work? I am right to assume that we can't have developers store internal values that we might change between two release patches, breaking the restore logic?

Well that's the reason to expose the state, so we allow users to persist and restore their settings for columns...
After yes we are going to have to migrate states between changes...
Happy to close the PR if you don't want it

@oliviertassinari
Copy link
Member

What about we isolate the private state from the public state? For instance in https://codesandbox.io/s/material-demo-forked-0n563?file=/demo.js, developers would end-up sending a lot of unneeded data through the network, e.g. send the rows multiple times, which doesn't scale for large datasets.

@dtassone
Copy link
Member Author

What about we isolate the private state from the public state? For instance in https://codesandbox.io/s/material-demo-forked-0n563?file=/demo.js, developers would end-up sending a lot of unneeded data through the network, e.g. send the rows multiple times, which doesn't scale for large datasets.

I would leave it this way for now

  • You don't need to restore the full state, as we do a merge internally
  • We won't document it or we will put a warning that this can change
  • We spend enough time on the state
  • This is just the first version we can improve it later
  • It is used internally so breaking it in pieces would mean another set of big refactoring, and state should only be stored in 1 place. (Single source of truth)

@oliviertassinari oliviertassinari changed the title [DataGrid] Introduce public state api [DataGrid] Prepare work for a future public state api Nov 10, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Ok, I have updated the title of the pull request, it's internal, for now, but it sets the stone in the right direction.

packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Show resolved Hide resolved
packages/grid/x-grid/src/XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Nov 11, 2020
dtassone and others added 11 commits November 11, 2020 20:51
fix api

finish state api

clean

cleanup

fix cylce dependency

added more tests

fix issues with state

more test, fix sourcemaps

Update packages/grid/_modules_/grid/models/api/stateApi.ts

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Add arg assert on test, move to DataGrid

clean
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants