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

[data-grid] Terminology for various row listings #4151

Closed
flaviendelangle opened this issue Mar 10, 2022 · 16 comments
Closed

[data-grid] Terminology for various row listings #4151

flaviendelangle opened this issue Mar 10, 2022 · 16 comments

Comments

@flaviendelangle
Copy link
Member

flaviendelangle commented Mar 10, 2022

Related to #3717

The grid contains more than half a dozen row listings depending on what the user (or our team) is trying to achieve.
In v5, some of those listings were not consistently named, one of the goal of v6 is to clarify as much as possible that part.

Proposals

  • Decide one terminology for each listing above and use this terminology in the name of every selector / api method / params property that uses this listing

  • Remove apiRef.current.getRowIndex, keep apiRef.current.getRowIndexRelativeToVisibleRows and maybe create a apiRef.current.getRowIndexRelativeToExpandedRows (see below to discuss the naming of the "expanded" row listing)

  • Remove all the api methods that are just calling a selector (apiRef.current.getAllRowIds for instance)

Existing listings

Rows in the order given by the user

Terminology in v5: rows
Terminology in v6: rows

Usages

  • gridRowIdsSelector

  • apiRef.current.getRowsCount

  • apiRef.current.getAllRowIds (delete in v6, can use gridRowIdsSelector)

Rows after the sorting

Terminology in v5: sorted rows
Terminology in v6: sorted rows

Usages

  • gridSortedRowIdsSelector

  • gridSortedRowEntriesSelector

  • apiRef.current.getSortedRows (delete in v6, can use gridSortedRowEntriesSelector)

  • apiRef.current.getSortedRowIds (delete in v6, can use gridSortedRowIdsSelector)

  • apiRef.current.getRowIdFromRowIndex (very unclear, delete in v6, can any row id selector to get the index from the wanted row listing)

Rows after the sorting and filtering

Terminology in v5: filtered sorted rows
Terminology in v6: filtered sorted rows (I keep the sorted because we could have a filtered non-sorted row listing)

  • gridFilteredSortedRowIdsSelector
  • gridFilteredSortedRowEntriesSelector

Rows after the sorting and filtering + without the collapsed children

Terminology in v5: visible rows
Terminology in v6: expanded rows ? sorted expanded rows ? non collapsed rows ? all page rows ?

If the tree is flat, this listing is equal to "Rows after the sorting and filtering".
But they must be clearly distinguished because when we have row children, depending on the use case we must use one or another.
For instance the keyboard navigation uses this listing whereas some exports use the filtered rows to include the collapsed children.

Usages

  • gridVisibleSortedRowIdsSelector (to rename)
  • gridVisibleSortedRowEntriesSelector (to rename)
  • gridVisibleRowCountSelector (to rename)
  • apiRef.current.getVisibleRowModels (delete in v6, can use gridVisibleSortedRowEntriesSelector, the Map return format is to specific for a dedicated methid)

Top level rows after the sorting and tiltering

If the tree is flat, this listing is equal to "Rows after the sorting and filtering".

Terminology in v5: visible sorted top level rows
Terminology in v6: filtered sorted top level rows ?

  • gridVisibleSortedTopLevelRowEntriesSelector (rename gridFilteredSortedTopLevelRowEntriesSelector)
  • gridVisibleTopLevelRowCountSelector (rename gridFilteredTopLevelRowCountSelector)

Rows that can be reaching by scrolling

If pagination is enabled, this is equivalent to the "Rows of the current page"
If pagination is disabled, this is equivalent to the "Rows after the sorting and filtering + without the collapsed children"

Terminology in v5: current page / visible rows
Terminology in v6: visible rows

Usages

  • apiRef.current.getRowIndexRelativeToVisibleRows
  • GridRowScrollEndParas["virtualRowsCount"] (rename visibleRowsCount in v6)

Rows of the current page

Should never be used when we don't know if pagination is enabled because otherwise it uses the default page / page size in state

Usages

  • gridPaginatedVisibleSortedGridRowEntriesSelector
  • gridPaginatedVisibleSortedGridRowIdsSelector
  • gridPaginationRowRangeSelector
@flaviendelangle
Copy link
Member Author

@mui/x if you have a moment to read and propose changes.

For #3882, I need the terminology of "Rows after the sorting and filtering + without the collapsed children", which is for me the biggest terminology pain right now.

@m4theushw
Copy link
Member

m4theushw commented Mar 11, 2022

apiRef.current.getRowIdFromRowIndex (very unclear, delete in v6, can any row id selector to get the index from the wanted row listing)

Its name could be the inverse of getRowIndexRelativeToVisibleRows, e.g. getRowIdFromIndexRelativeToVisibleRows. I remember this method was used in the keyboard navigation but it didn't work with filtering, so we created the custom logic below:

const rowIndexBefore = visibleSortedRows.findIndex((row) => row.id === params.id);

Rows after the sorting and filtering + without the collapsed children

Possible confusion with the detail panel? I also used "expanded". Although, all selectors have the feature name as prefix, e.g. gridDetailPanelExpandedRowIdsSelector. Should we include the feature name in these selectors or mention that they are aware of the tree structure of the rows?

  • No prefix -> fetches all data that exists in state

  • Has prefix -> fetches all data from the state but which is also affected by a given feature

@flaviendelangle
Copy link
Member Author

Its name could be the inverse of getRowIndexRelativeToVisibleRows, e.g. getRowIdFromIndexRelativeToVisibleRows. I remember this method was used in the keyboard navigation but it didn't work with filtering, so we created the custom logic below:

We could indeed replace the current method with one that uses the "visible rows" (v6 terminology).


Possible confusion with the detail panel?

Do we consider the detail panel as a row ? If not I guess the confusion should not be problematic.

Which terminology would you use for this row listing.

@alexfauquette
Copy link
Member

alexfauquette commented Mar 15, 2022

Do we consider the detail panel as a row? If not I guess the confusion should not be problematic.

I understand that it could be confusing, but the presence of "Rows" in the selector name should be enough to remove the confusion


About the section "Rows that can be reached by scrolling" there is a missing edge case about tree data + pagination

If pagination is enabled, this is equivalent to the "Rows of the current page"

Visible in its definition of "in Page" + "pass Filtered" + "expended row" should be equivalent to "Rows of the current page + without the collapsed children"

Do we have use cases where it would be nice to have a selector for rows that are on the page but not expanded?

If yes, we could introduce InPage such that we would have

  • InPage for the rows in the page (it implies sorted+filterred)
  • visible for all the criteria: InPage + Expanded + Filitered + Sorted

@flaviendelangle
Copy link
Member Author

About the section "Rows that can be reached by scrolling" there is a missing edge case about tree data + pagination

Could you elaborate ?

Visible in its definition of "in Page" + "pass Filtered" + "expended row" should be equivalent to "Rows of the current page + without the collapsed children"

For me it is exactly the case

Do we have use cases where it would be nice to have a selector for rows that are on the page but not expanded?

The selector do not exist right now and I never heard of a use case.
But I agree that it could become useful someday.

@alexfauquette
Copy link
Member

What I called the "missing edge case about tree data + pagination" is the selector to get page rows, including collapsed ones.

I was checking that all the meaning full combinations are in the list to be sure the naming convention will allow to add them later if needed


About visible sorted top level rows becoming filtered sorted top level rows it could be top level rows

If we assume selectors are only to get information about the interface state we could remove mentions of filtered and sorted. Except rows and sorted rows all the selectors mentioned in this issue are applying filtered sorted

@flaviendelangle
Copy link
Member Author

What I called the "missing edge case about tree data + pagination" is the selector to get page rows, including collapsed ones.

OK so if it is for all pages, we have gridFilteredSortedRowIdsSelector
And for the current page, it does not exist (related to my previous message)

About visible sorted top level rows becoming filtered sorted top level rows it could be top level rows

You could also have a listing of the unsorted and/or unfiltered top level rows.
But maybe it's such an edge case that we should not take it into account

@m4theushw
Copy link
Member

Do we consider the detail panel as a row ? If not I guess the confusion should not be problematic.

The detail panel is not a row, but the row from the detail panel also has the ability to expand. In this context, someone may see a selector named with "expanded" and think it works with the detail panel but it doesn't.

Which terminology would you use for this row listing.

I would include in the name the origin of the information. Take as example gridVisibleRowCountSelector and gridVisibleTopLevelRowCountSelector. To me, "top level rows" is not 100% clear. But if look at how these selectors are defined I see that the latter depends on gridRowTreeSelector so I automatically link it to tree data. My suggestion is to use "tree" as a prefix in selectors that get information from rows.tree, so gridVisibleTopLevelRowCountSelector becomes gridTreeVisibleTopLevelRowCountSelector.

@flaviendelangle
Copy link
Member Author

so I automatically link it to tree data.

Not just tree data, also row grouping

If we take gridTreeVisibleTopLevelRowCountSelector then how do you name the current gridVisibleSortedRowIdsSelector ?
Because it does get information from rows.tree to know what is expanded or not and not include the collapsed children.
gridTreeExpandedFilteredSortedRowIdsSelector ? gridTreeExpandedRowIdsSelector ?

@m4theushw
Copy link
Member

m4theushw commented Mar 15, 2022

Because it does get information from rows.tree to know what is expanded or not and not include the collapsed children.
gridTreeExpandedFilteredSortedRowIdsSelector

Where it uses rows.tree?

gridVisibleSortedRowIdsSelector -> gridFilteredSortedRowIdsSelector? The concept of "visible" is "reachable by scroll" but we don't know yet if pagination is enabled or not.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Mar 15, 2022

gridVisibleSortedRowIdsSelector and gridFilteredSortedRowIdsSelector are two distincts listings.
They have equal content when there is no children (tree data off and no grouping criteria)
BUT they differ when there is children.

gridVisibleSortedRowIdsSelector:

The current gridVisibleSortedRowIdsSelector uses visible in its previous meaning and should be renamed.
Its content is the rows of all pages, post sorting + filtering and without the collapsed children.
It cares about the expansion status.

gridFilteredSortedRowIdsSelector:

The current gridFilteredSortedRowIdsSelector is the rows of all pages, post sorting + filtering, BUT with the collapsed children.
It does not care about the expansion status.


The relation with rows.tree is in filterRowTreeFromTreeData and filterRowTreeFromGroupingColumns.
We build the filteredRowsLookup without caring about the expansion and visibleRowsLookup caring about the expansion (visibleRowsLookup should be renamed accordingly to the new nomenclature).
So the selector itself does not access rows.tree but the data in the state itself has been built using rows.tree.


gridFilteredSortedRowIdsSelector is rightly named I think
But gridVisibleSortedRowIdsSelector must be renamed because it uses "visible" with an outdated meaning.

And from what I understand of your logic, it should have tree in its name.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Mar 15, 2022

gridVisibleSortedRowIdsSelector is being used for anything related to what is displayed on the screen (virtualization / keyboard navigation, selection, infinite loading, variable row height...).

useGridVisibleRows uses it behind the scene (if pagination is off it returns directly gridVisibleSortedRowEntriesSelector and if pagination is on, it returns gridPaginatedVisibleSortedGridRowEntriesSelector which returns a slice of gridVisibleSortedRowEntriesSelector.

So anything using useGridVisibleRows is indirectly using gridVisibleSortedRowIdsSelector


gridFilteredSortedRowIdsSelector has been added recently and is used for the exports when users wants to export the children even if they are collapsed.

@m4theushw
Copy link
Member

I feel like only renaming gridVisibleSortedRowIdsSelector won't solve the confusion. When tree data and row grouping is not in use, the selectors make sense, but once only one of these features is enabled other selectors need to be used. Maybe here's the limit where we can say to users to use the selectors to access the data. Adding a method to the API with a param where the developer can decide if he wants the children or not seems much simpler than trying to build a future-proof naming strategy.

@flaviendelangle
Copy link
Member Author

When tree data and row grouping is not in use, the selectors make sense, but once only one of these features is enabled other selectors need to be used

I don't understand your point here.
Which selectors have to change ?
The selectors are built with the tree in mind, and if used without any children, they still work.
We don't have a single "if tree then selectorA else selectorB" in our codebase.

We can add a method if it's easier for users, which would probably be a method on useGridRowGrouping and useGridTreeData because the rows the user actually wants are not the same (on the row grouping they will want the leaves, on the tree data is kind of unclear, if they will want the leaves or the non-auto-generated rows, or all the rows, the selectors have the advantage of being very flexible here).
But we still have to get a good naming strategy for our internal usage.
And for me the dichotomy is not "with our without children" but "with or without collapsed rows". For the

@m4theushw
Copy link
Member

m4theushw commented Mar 25, 2022

And for me the dichotomy is not "with our without children" but "with or without collapsed rows".

Yeah, but for developers, I think the best way is to expose a method to get the rows and to have a param specifying if the collapsed rows should be fetched or not. In the past, it was a mess to get the currently visible rows then useGridVisibleRows was added and solved the problem.

For selectors, I tried to come up with the following terminology, but for it to work we also need to update the state. filter.visibleRowsLookup contains the rows that satisfy the filtering criteria and are also expanded, but "visible" doesn't include the pagination so it violates the concept of "reachable by scroll". I would first move it to something like rows.expansionStatus storing only if the row is expanded or not. Then to get the filtered + expanded we can intersect rows.expansionStatus with filter.filteredRowsLookup. Doing this change, the terminology might work:

  • Paginated: includes only the rows on the current page
  • Filtered: includes only the rows that satisfy the filtering criteria
  • Expanded: includes only the expanded rows
  • Sorted: includes the rows considering the current sorting

I avoided the "visible" term because it works for methods but not for selectors.

One resulting selector could be gridExpandedFilteredSortedRowIdsSelector = same content from gridFilteredSortedRowIdsSelector and can be used for exporting. One tradeoff is that selectors that didn't mention "expanded" now will have.

When combining prefixes, the first value used goes to the end of the selector name. gridExpandedFilteredSortedRowIdsSelector first needs the sorted rows, then from these rows, it keeps only the filtered ones, and finally removes those not expanded. If it was gridPaginatedExpandedFilteredSortedRowIdsSelector, then the slicing (for pagination) would be the last step.

@flaviendelangle
Copy link
Member Author

think the best way is to expose a method to get the rows and to have a param specifying if the collapsed rows should be fetched or not

We'll see what method(s) would be the most useful. If a use-case is very common and a method makes it simpler, then of course I am in favor of a method (like the useGridVisibleRows).

The nomenclature is your are describing looks good to me.

@flaviendelangle flaviendelangle changed the title [DataGrid] Terminology for various row listings [data-grid] Terminology for various row listings Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants