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

WindowScroller imperative scrolling #540

Merged
merged 16 commits into from
Nov 21, 2024

Conversation

herschelrs
Copy link
Contributor

Adds imperative scrolling api to WindowScroller. Only react bindings implemented, also no tests written.

@inokawa
Copy link
Owner

inokawa commented Nov 10, 2024

#495

@herschelrs
Copy link
Contributor Author

That issue was requesting a two-dimensional windowscroller with imperative scrolling--which you said presents problems. This PR doesn't solve that problem but currently there's not even a one dimensional window scroller with imperative scrolling, so this is surely an improvement?

(If you're likely to merge I'll fix the errors)

Copy link
Owner

@inokawa inokawa left a comment

Choose a reason for hiding this comment

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

Hi, could you add some e2e tests for scrollToIndex like this?

virtua/e2e/VList.spec.ts

Lines 596 to 868 in 231fb7e

test.describe("check if scrollToIndex works", () => {
test.beforeEach(async ({ page }) => {
await page.goto(storyUrl("basics-vlist--scroll-to"));
});
test.describe("align start", () => {
test("mid", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("700");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const firstItem = await getFirstItem(component);
await expect(firstItem.text).toEqual("700");
await expect(firstItem.top).toEqual(0);
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("650");
await expect(await component.innerText()).not.toContain("750");
});
test("start", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("500");
await button.click();
await component.waitForElementState("stable");
await expect(await component.innerText()).toContain("500");
await clearInput(input);
await input.fill("0");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const firstItem = await getFirstItem(component);
await expect(firstItem.text).toEqual("0");
await expect(firstItem.top).toEqual(0);
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("50\n");
});
test("end", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("999");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const lastItem = await getLastItem(component);
await expect(lastItem.text).toEqual("999");
expectInRange(lastItem.bottom, { min: -0.9, max: 1 });
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("949");
});
test("mid smooth", async ({ page, browserName }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
await page.getByRole("checkbox", { name: "smooth" }).click();
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
const scrollListener = listenScrollCount(component);
await clearInput(input);
await input.fill("700");
await button.click();
await page.waitForTimeout(500);
const called = await scrollListener;
// Check if this is smooth scrolling
await expect(called).toBeGreaterThanOrEqual(
// TODO find better way to check in webkit
browserName === "webkit" ? 2 : 10
);
// Check if scrolled precisely
const firstItem = await getFirstItem(component);
await expect(firstItem.text).toEqual("700");
expectInRange(firstItem.top, { min: 0, max: 1 });
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("650");
await expect(await component.innerText()).not.toContain("750");
});
});
test.describe("align end", () => {
test.beforeEach(async ({ page }) => {
await page.getByRole("radio", { name: "end" }).click();
});
test("mid", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("700");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const lastItem = await getLastItem(component);
await expect(lastItem.text).toEqual("700");
expectInRange(lastItem.bottom, { min: 0, max: 1 });
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("650");
await expect(await component.innerText()).not.toContain("750");
});
test("start", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("500");
await button.click();
await component.waitForElementState("stable");
await expect(await component.innerText()).toContain("500");
await clearInput(input);
await input.fill("0");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const firstItem = await getFirstItem(component);
await expect(firstItem.text).toEqual("0");
await expect(firstItem.top).toEqual(0);
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("50\n");
});
test("end", async ({ page }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
await clearInput(input);
await input.fill("999");
await button.click();
await component.waitForElementState("stable");
// Check if scrolled precisely
const lastItem = await getLastItem(component);
await expect(lastItem.text).toEqual("999");
expectInRange(lastItem.bottom, { min: 0, max: 1 });
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("949");
});
test("mid smooth", async ({ page, browserName }) => {
const component = await getScrollable(page);
await component.waitForElementState("stable");
// check if start is displayed
await expect((await getFirstItem(component)).text).toEqual("0");
await page.getByRole("checkbox", { name: "smooth" }).click();
const button = page.getByRole("button", { name: "scroll to index" });
const input = await button.evaluateHandle(
(el) => el.previousSibling as HTMLInputElement
);
const scrollListener = listenScrollCount(component);
await clearInput(input);
await input.fill("700");
await button.click();
await page.waitForTimeout(500);
const called = await scrollListener;
// Check if this is smooth scrolling
await expect(called).toBeGreaterThanOrEqual(
// TODO find better way to check in webkit
browserName === "webkit" ? 2 : 10
);
// Check if scrolled precisely
const lastItem = await getLastItem(component);
await expect(lastItem.text).toEqual("700");
expectInRange(lastItem.bottom, { min: 0, max: 1 });
// Check if unnecessary items are not rendered
await expect(await component.innerText()).not.toContain("650");
await expect(await component.innerText()).not.toContain("750");
});
});
});

src/react/WindowVirtualizer.tsx Outdated Show resolved Hide resolved
src/react/WindowVirtualizer.tsx Outdated Show resolved Hide resolved
@herschelrs herschelrs force-pushed the windowscroller-imperative-scrolling branch from 0945a68 to d33c079 Compare November 15, 2024 18:48
@herschelrs herschelrs closed this Nov 16, 2024
@herschelrs herschelrs reopened this Nov 16, 2024
@herschelrs
Copy link
Contributor Author

herschelrs commented Nov 16, 2024

(closed by mistake)
Should be good now.

One thing I was uncertain about, I added viewportAdjustment to account for scrollbar size in _scrollToIndex--this was simpler for now than making further changes to VirtualStore, but maybe isn't desirable.

edit: I'm a bit confused what triggers github actions to run the tests again, but they passed locally.

@inokawa
Copy link
Owner

inokawa commented Nov 17, 2024

Thank you! I'll take a look later.

Triggering GitHub Actions needs my confirmation, because it's default setting of GitHub aiming to ignore spam like things.

@inokawa inokawa force-pushed the windowscroller-imperative-scrolling branch from 46e03a7 to 7d4adbf Compare November 18, 2024 12:37
@inokawa inokawa self-requested a review November 18, 2024 13:14
@herschelrs
Copy link
Contributor Author

Sorry I didn't catch this, I think I made a mistake when I was checking what was going on with startMargin.

Copy link
Owner

@inokawa inokawa left a comment

Choose a reason for hiding this comment

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

Sorry for may late reply. I did some refactor for my preference after that.

scrollToIndex seems to be working fine. Thank you for your contribution.

@inokawa inokawa merged commit 3f0881e into inokawa:main Nov 21, 2024
5 checks passed
@inokawa
Copy link
Owner

inokawa commented Nov 21, 2024

Released in 0.37.4.

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.

2 participants