Skip to content

Conversation

@spmonahan
Copy link
Contributor

@spmonahan spmonahan commented Sep 18, 2023

Previous Behavior

scrollToIndex() for a GroupedList (or grouped DetailsList -- GroupedList is used under the hood in this scenario) does not always scroll to the provided index.

New Behavior

scrollToIndex() now works.

The reason this did not work is that GroupedList is actually List of Lists and internally it maintains two lists:

  1. items a flat list of items
  2. groups a nested list of groups that items is projected into

In GroupedList the root List, which is the list that scrollToIndex() is called only has groups.length items in it (where groups.length is the number of root level groups) and in many (most?) cases items.length > groups.length. When this is the case any item index > groups.length is out of bounds and the list is not scrolled because the item in question cannot be found.

This is an example of scrollToIndex working because items.length < groups.length.

This change updates the scrollToIndex() implementation in GroupedList to map the provided index back to a group index which is guaranteed to be in bounds since this method is only called on the root list in a GroupedList.

NOTE: this issue does not affect GroupedListV2 as that is implemented as a flat list.

A similar API, focusIndex() does not exhibit this problem. The reason for this is that it looks up the item in question and calls focus() on it which triggers the browser to bring the element in to view.

Related Issue(s)

scrollToIndex accepts an item index value which, for a flat List
is the row index in the List. Because GroupedList is a List of Lists
the item index does not necessarily correspond with the row index in
the List. In fact, the first level of the List only has items for the
number of groups in the List so in most cases the scroll to index will
be out of bounds so far as the List is concerned and therefore will not
be found.

This change remaps "item index" to "group index" in GroupedLists
scrollToIndex() method which should ensure the group that owns an
particular row is scrolled in to view.

Note: this issue does not affect GroupedListV2 as it is implemented
as a flat List under the hood.
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 51d8ffd:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

🕵 fluentuiv8 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

📊 Bundle size report

🤖 This report was generated against 6840cead921e4d30772dd9ab2cd234fe26a690f4

@size-auditor
Copy link

size-auditor bot commented Sep 18, 2023

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-ShimmeredDetailsList 235.715 kB 235.96 kB ExceedsBaseline     245 bytes
office-ui-fabric-react fluentui-react-GroupedList 130.546 kB 130.791 kB ExceedsBaseline     245 bytes
office-ui-fabric-react fluentui-react-DetailsList 224.963 kB 225.208 kB ExceedsBaseline     245 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 6840cead921e4d30772dd9ab2cd234fe26a690f4 (build)

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 627 626 5000
Breadcrumb mount 1665 1667 1000
Checkbox mount 1671 1692 5000
CheckboxBase mount 1443 1449 5000
ChoiceGroup mount 2980 2881 5000
ComboBox mount 649 637 1000
CommandBar mount 6145 6123 1000
ContextualMenu mount 12408 12397 1000
DefaultButton mount 755 761 5000
DetailsRow mount 2151 2167 5000
DetailsRowFast mount 2134 2126 5000
DetailsRowNoStyles mount 1973 1983 5000
Dialog mount 2589 2785 1000
DocumentCardTitle mount 242 229 1000
Dropdown mount 1981 1981 5000
FocusTrapZone mount 1117 1153 5000
FocusZone mount 1052 1093 5000
GroupedList mount 41116 41254 2
GroupedList virtual-rerender 19789 19692 2
GroupedList virtual-rerender-with-unmount 50154 50238 2
GroupedListV2 mount 233 227 2
GroupedListV2 virtual-rerender 221 215 2
GroupedListV2 virtual-rerender-with-unmount 218 220 2
IconButton mount 1082 1074 5000
Label mount 329 327 5000
Layer mount 2674 2664 5000
Link mount 387 380 5000
MenuButton mount 923 934 5000
MessageBar mount 21286 21255 5000
Nav mount 1903 1944 1000
OverflowSet mount 770 775 5000
Panel mount 1773 1763 1000
Persona mount 724 759 1000
Pivot mount 869 848 1000
PrimaryButton mount 823 827 5000
Rating mount 4490 4592 5000
SearchBox mount 885 886 5000
Shimmer mount 1846 1815 5000
Slider mount 1310 1265 5000
SpinButton mount 2869 2864 5000
Spinner mount 385 377 5000
SplitButton mount 1765 1792 5000
Stack mount 388 410 5000
StackWithIntrinsicChildren mount 845 835 5000
StackWithTextChildren mount 2583 2586 5000
SwatchColorPicker mount 6039 6162 5000
TagPicker mount 1429 1456 5000
Text mount 362 371 5000
TextField mount 938 920 5000
ThemeProvider mount 820 820 5000
ThemeProvider virtual-rerender 573 579 5000
ThemeProvider virtual-rerender-with-unmount 1260 1269 5000
Toggle mount 603 632 5000
buttonNative mount 191 192 5000

@spmonahan
Copy link
Contributor Author

Closing this in favor of #29332.

@spmonahan spmonahan closed this Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GroupedList (and grouped DetailsList) do not scrollToIndex()

4 participants