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

Use SliverList and cache content height #1802

Merged
merged 4 commits into from
May 12, 2023
Merged

Conversation

QuncCccccc
Copy link
Contributor

Fixes #1694
This PR is to fix the shaking scrollbar problem. We replace the ListViews with CustomScrollView and cache the content height to provide a better estimation for the max scroll offset.

Same changes are applied to both Material 3 demo in the root directory and the M3 demo in the /experimental/

Screen.Recording.2023-05-10.at.4.05.27.PM.mov
two_cols.mov

Pre-launch Checklist

  • I read the [Flutter Style Guide] recently, and have followed its advice.
  • I signed the [CLA].
  • I read the [Contributors Guide].
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

@domesticmouse
Copy link
Contributor

You'll need to pull from main to have a chance at getting a (mostly) green CI.

Copy link

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 118 to 121
// Based on the fact that the list content is fixed, we cache
// the height of the content during the first-time scrolling.
// The cached height can be used to override
// `SliverChildDelegate.estimateMaxScrollOffset`, to avoid a shaking scrollbar.

Choose a reason for hiding this comment

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

Suggested change
// Based on the fact that the list content is fixed, we cache
// the height of the content during the first-time scrolling.
// The cached height can be used to override
// `SliverChildDelegate.estimateMaxScrollOffset`, to avoid a shaking scrollbar.
// If the content of a CustomScrollView does not change, then it's
// safe to cache the heights of each item as they are laid out. The
// sum of the cached heights are returned by an override of
// `SliverChildDelegate.estimateMaxScrollOffset`. The default version
// of this method bases its estimate on the average height of the
// visible items. The override ensures that the scrollbar thumb's
// size, which depends on the max scroll offset, will shrink smoothly
// as the contents of the list are exposed for the first time, and
// then remain fixed.

],
),
);
}
}

// Based on the fact that the list content is fixed, we cache

Choose a reason for hiding this comment

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

See suggested rewrite for the experimental version of this comment

@QuncCccccc QuncCccccc merged commit 1d6e5a0 into main May 12, 2023
13 of 14 checks passed
@parlough parlough deleted the refactor_list_view_M3_demo branch May 12, 2023 18:08
bisor0627 added a commit to bisor0627/samples that referenced this pull request Jun 6, 2023
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.

Consider refactoring component lists into SliverLists
3 participants