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

refactor: make virtualizer use dynamic placeholder height #3908

Merged
merged 2 commits into from
May 23, 2022

Conversation

tomivirkki
Copy link
Member

Part of vaadin/flow-components#3206

Using the static value of 200px for the placeholder height doesn't play well when the actual item height is much smaller.

Note: this is only an issue in the rare cases where the items have the intrinsic height of 0 after an update and then asynchronously increase in height.

This PR updates Virtualizer to dynamically update the placeholder height to be the average of the 10 previous actual item heights. Having the placeholder height be as close as possible to the actual item heights results in much nicer scrolling UX:

Static placholder height (of 200px):

static-placeholder-height.mp4

Dynamic placeholder height:

dynamic-placeholder-height.mp4

Comment on lines -304 to +336
// After running super._scrollHandler, fix _virtualStart to workaround an iron-list issue.
// See https://github.com/vaadin/web-components/issues/1691
const reusables = this._getReusables(true);
this._physicalTop = reusables.physicalTop;
this._virtualStart += reusables.indexes.length;
this._physicalStart += reusables.indexes.length;
const isScrollingDown = delta >= 0;
const reusables = this._getReusables(!isScrollingDown);

if (reusables.indexes.length) {
// After running super._scrollHandler, fix internal properties to workaround an iron-list issue.
// See https://github.com/vaadin/web-components/issues/1691
this._physicalTop = reusables.physicalTop;

if (isScrollingDown) {
this._virtualStart -= reusables.indexes.length;
this._physicalStart -= reusables.indexes.length;
} else {
this._virtualStart += reusables.indexes.length;
this._physicalStart += reusables.indexes.length;
}
this._resizeHandler();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not directly related to this change: a forward-port of vaadin/vaadin-grid#2217 to fix some tests that started failing

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tomivirkki tomivirkki requested a review from web-padawan May 23, 2022 06:57
@web-padawan web-padawan merged commit 83ebcc1 into master May 23, 2022
@web-padawan web-padawan deleted the virtualizer-dynamic-placeholder-height branch May 23, 2022 07:19
@vaadin-bot
Copy link
Collaborator

Hi @tomivirkki , this commit cannot be picked to 23.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick 83ebcc1
error: could not apply 83ebcc1... refactor: make virtualizer use dynamic placeholder height (#3908)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@tomivirkki
Copy link
Member Author

this commit cannot be picked to 23.0 by this bot, can you take a look and pick it manually?

Backporting this to earlier minors shouldn't be necessary, since pretty much the only affected component is the Flow VirtualList and only in the case if the default placeholder height is removed (which probably will not happen for earlier minors).

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.1.0.rc1 and is also targeting the upcoming stable 23.1.0 version.

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.

3 participants