[DataGrid] Core api refactoring #130

dtassone · 2020-07-29T17:11:19Z

No description provided.

dtassone · 2020-07-29T17:15:21Z

Regression to look at: Sorting issue on click on header

oliviertassinari

Regression to look at: Sorting issue on click on header

This sounds like a great opportunity for a new test case :).

oliviertassinari · 2020-07-29T19:04:54Z

packages/storybook/src/stories/playground/real-data-demo.stories.tsx

@@ -34,7 +34,7 @@ const getGridOptions: () => GridOptionsProp = () => {
      action('onSelectionChange', {
        depth: 1,
      })(params),
-    onSortedColumns: (params) => action('onSortedColumns')(params),
+    onColumnsSorted: (params) => action('onColumnsSorted')(params),


?

Suggested change

onColumnsSorted: (params) => action('onColumnsSorted')(params),

onColumnsSort: (params) => action('onSortedColumns')(params),

The past it to represent that the action occurs after the sorting has taken place.
The before event is onColumnsSortingChange

Oh OK, I didn't spot the difference, I thought they were two events for two different states, I didn't realize it was the same state but events raised at different point in time 😇.

What do you think about using two closed names?

onColumnsSortingChange

onColumnsSortingChanged

ATM we have

onColumnsSortingChange

onColumnsSorted

The 2 names suggested are too similar, it can be confusing I think.

How about we follow the c# convention.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members

✔️ DO give events names with a concept of before and after, using the present and past tenses.
For example, a close event that is raised before a window is closed would be called Closing, and one that is raised after the window is closed would be called Closed.

So in our case, we could do

onColumnsSortingChanging or onColumnsSortedChanging, onColumnsSortChanging, onSortChanging

onColumnsSortingChanged or onColumnsSortedChanged, onColumnsSortChanged, onSortChanged

Should we introduce the concept all around? So for pages it would be:

onPageChanging & onPageChanged

What if we don't have or see the need for before / after notion in a particular event?
Should we just do it?

How about click?
onCellClick => onCellClicking, onCellClicked 🤔

c# convention

I think that we can take inspiration from the outside as long as it doesn't conflict with the convention that React has. React should always take precedence. React doesn't leave us much choice for the controlled name, it should end with "change".

For the post action, name, do we even need it? Do we have a practical use case. As far as I can push the reflection, I see two cases:

The data is sorted client-side, there are no delays, onColumnsSorted has no use cases.

The data is sorted server-side, the user already controls when the data is loaded and hence sorted, grouped, filtered, etc.

keeping only onSortingChange simple and nice what do you think?

I love the direction. The next elements I wonder about are:

What's the difference between the sortedColumns and sortModel state? Why do we need sortedColumns?
https://github.com/mui-org/material-ui-x/blob/4481b72cf03c6dcd0338d646d1cd6060a4d0b8f4/packages/grid/x-grid-modules/src/hooks/features/useSorting.ts#L120-L125

Assuming sortedColumns === sortModel what about we have ColumnSortedParams === SortModel?

What about the uncontrolled and controlled APIs?

Shouldn't we breakdown the sortModel option? As of now, it blends the uncontrolled and controlled behavior. You can change the sort model state without changing the prop (using the UI), and you can also change the prop to resync the sort model. I think that it will confuse developers.
We could have defaultSortModal for uncontrolled mode (+ refAPI usage for imperative calls) and sortModel for controlled mode?

Shouldn't we rename onSortingChange(ColumnSortedParams) to onSortModelChange(SortModel) to account for the controlled callback?

Assuming sortedColumns === sortModel what about we have ColumnSortedParams === SortModel?

the idea is to provide all the necessary arguments for the user to do everything it needs without looking to add other props, like in CellParams

What about the uncontrolled and controlled APIs?

Interesting point. I suggest we start with what we have now and we open an RFC for controlled/uncontrolled. And we can always gather feedbacks first and reassess the need of defaultX prop

Shouldn't we rename onSortingChange(ColumnSortedParams) to onSortModelChange(SortModel) to account

for the controlled callback?
Sure, happy with that, if we keep the prop sortModel

I was thinking of renaming the prop options.sortModel with options.sortBy what do you think?
So onSortingChange & sortBy: SortModel or onSortModelChange & sortModel: SortModel?

taking the later, it's more standard for now.
We can rename later when we clarify a bit more our conventions

I was thinking of renaming the prop options.sortModel with options.sortBy what do you think?

👍 I think that sortBy is more descriptive than sortModel. We better understand what the option aims to achieve, it sounds better. Should we open an issue to keep track of the idea?

the idea is to provide all the necessary arguments for the user to do everything it needs without looking to add other props, like in CellParams

No objection here, the reason I have asked is that in order to make the controllable API efficient, for instance https://next--material-ui.netlify.app/components/tree-view/#ControlledTreeView.tsx, you need to be able to wire things:

const [expanded, setExpanded] = React.useState([]); const handleToggle = (event, nodeIds) => { setExpanded(nodeIds); }; return ( <TreeView expanded={expanded} defaultSelected={[]} onNodeToggle={handleToggle}

P.S. Notice the onX and handleX naming convention. Notice on how we always expose event. Notice that if you control the component and remove the event feedback loop, nothing happens. What sortModel implements is the uncontrolled behavior with the handle change optional behavior (something an uncontrolled input supports but that we didn't in useControlled.js)

If you want to keep it, I would propose to have the state (sortModel or sortBy once renamed) as the first argument, and the helping values as a second argument, under an object.

Interesting point. I suggest we start with what we have now and we open an RFC for controlled/uncontrolled. And we can always gather feedbacks first and reassess the need of defaultX prop

Yeah, an RFC sounds like the way to go, we are already off-topic on the pull request!

Yes to open an issue with the sortBy.

oliviertassinari · 2020-07-29T19:06:14Z

packages/grid/x-grid-modules/src/hooks/root/useEvents.ts

+
+  const getEventParams = React.useCallback(
+    (event: any) => {
+      if (event.target == null) {


Shouldn't we throw if target is null rather than silencing the problem?

Well I don't think it can happen. If it does and we throw it can break the app 🤔

If you don't think that it can happen, I think that throwing will help us. No matter what happen, we will win.

If it throw, we would have learn something new about how the logic behaves. Help make better design decisions later on, or even spot bugs.

If it doesn't throw. We would have reduced the combinatory of the logic, reducing the number of cases we need to consider, which mean a simpler logic.

packages/grid/x-grid-modules/src/models/gridOptions.tsx

packages/storybook/src/stories/grid-sorting.stories.tsx

packages/grid/x-grid-modules/src/models/api/coreApi.ts

packages/storybook/src/documentation/pages/sorting.stories.mdx

oliviertassinari · 2020-07-29T21:01:25Z

packages/storybook/src/documentation/pages/sorting.stories.mdx

+const [rows, setRows] = React.useState<RowsProp>([]);
+const onColumnsSortingChange = (params) => {
+  sortServerRows(params).then((newRows) => {
+    setRows(newRows);


Let's treat this one separately.
Should we manage concurrency in demos? maybe an RFC, or doc feedback
There is no server here so I don't think there is any point...

Maybe we should do a demo with a real api? 🤔

concurrency in demos?

I think that the question is either or not developers can trust our demos, to behave bug-free, when they copy and paste them, making minor changes, and stress testing them.

it will be as we have a fixed timeout so, latest result will always be the correct one

oliviertassinari · 2020-07-29T21:02:30Z

packages/storybook/src/documentation/pages/sorting.stories.mdx

+## Server-side sorting
+
+XGrid supports both client and server-side pagination. By default, sorting works on the client-side. To switch it to server-side,
+set the property `sortingMode` to `server` as string or you can use our `FeatureModeConstant`.


IMHO, we don't need to export not document FeatureModeConstant it feels like using a string will be simpler, but no strong preference.

oliviertassinari · 2020-07-29T21:02:50Z

packages/storybook/src/stories/grid-sorting.stories.tsx

@@ -244,7 +251,21 @@ export const SortingWithFormatter = () => {
    </div>
  );
 };
+export const SortModelOptionsMultiple = () => {


Suggested change

export const SortModelOptionsMultiple = () => {

export function SortModelOptionsMultiple() {

oliviertassinari · 2020-07-29T21:03:07Z

packages/storybook/src/stories/grid-sorting.stories.tsx

@@ -317,3 +339,110 @@ export const SortedEventsApi = () => {
    </React.Fragment>
  );
 };
+export const SortedEventsOptions = () => {


Suggested change

export const SortedEventsOptions = () => {

export function SortedEventsOptions() {

packages/storybook/src/documentation/pages/demos/sorting/serverSorting.demo.tsx

oliviertassinari reviewed Jul 29, 2020

View reviewed changes

oliviertassinari changed the title ~~Core api refactoring~~ [DataGrid] Core api refactoring Jul 29, 2020

oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 29, 2020

oliviertassinari mentioned this pull request Jul 29, 2020

refactor sorting and add server side sorting #129

Merged

oliviertassinari reviewed Jul 30, 2020

View reviewed changes

packages/storybook/src/documentation/pages/demos/sorting/serverSorting.demo.tsx Outdated Show resolved Hide resolved

dtassone added 2 commits July 30, 2020 12:06

refactor split useApi

34ad42b

fix click event issue

82b4cf7

dtassone force-pushed the coreApiRefactoring branch from 6155dd6 to 82b4cf7 Compare July 30, 2020 10:38

dtassone added 2 commits July 30, 2020 12:48

add throw if event target null

9fa893f

refactored sorting events

1bb9eaa

dtassone requested a review from oliviertassinari July 30, 2020 14:42

oliviertassinari approved these changes Jul 30, 2020

View reviewed changes

fix useState in severSorting demo

be18018

oliviertassinari mentioned this pull request Jul 30, 2020

[DataGrid] Rename sortModel to sortBy #133

Closed

dtassone merged commit 6934842 into mui:master Jul 30, 2020

oliviertassinari mentioned this pull request Apr 13, 2021

[core] Remove dead code #1405

Closed

Vansh1190 mentioned this pull request Jul 10, 2024

[charts] Page Broke in React Bar Chart #13793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Core api refactoring #130

[DataGrid] Core api refactoring #130

dtassone commented Jul 29, 2020

dtassone commented Jul 29, 2020

oliviertassinari left a comment

oliviertassinari Jul 29, 2020

dtassone Jul 30, 2020

oliviertassinari Jul 30, 2020 •

edited

Loading

dtassone Jul 30, 2020

oliviertassinari Jul 30, 2020 •

edited

Loading

oliviertassinari Jul 30, 2020 •

edited

Loading

dtassone Jul 30, 2020 •

edited

Loading

dtassone Jul 30, 2020

oliviertassinari Jul 30, 2020

dtassone Jul 30, 2020

oliviertassinari Jul 29, 2020

dtassone Jul 30, 2020

oliviertassinari Jul 30, 2020

oliviertassinari Jul 29, 2020

dtassone Jul 30, 2020

oliviertassinari Jul 30, 2020 •

edited

Loading

dtassone Jul 30, 2020

oliviertassinari Jul 29, 2020

oliviertassinari Jul 29, 2020

oliviertassinari Jul 29, 2020

	onColumnsSorted: (params) => action('onColumnsSorted')(params),
	onColumnsSort: (params) => action('onSortedColumns')(params),

	export const SortModelOptionsMultiple = () => {

	export function SortModelOptionsMultiple() {

	export const SortedEventsOptions = () => {

	export function SortedEventsOptions() {

[DataGrid] Core api refactoring #130

[DataGrid] Core api refactoring #130

Conversation

dtassone commented Jul 29, 2020

dtassone commented Jul 29, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

dtassone Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jul 30, 2020 •

edited

Loading

oliviertassinari Jul 30, 2020 •

edited

Loading

oliviertassinari Jul 30, 2020 •

edited

Loading

dtassone Jul 30, 2020 •

edited

Loading

oliviertassinari Jul 30, 2020 •

edited

Loading