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

Feat: ScrollScreenshot #1010

Merged
merged 9 commits into from
Feb 3, 2024
Merged

Feat: ScrollScreenshot #1010

merged 9 commits into from
Feb 3, 2024

Conversation

zbysir
Copy link
Contributor

@zbysir zbysir commented Feb 2, 2024

Add ScrollScreenshot, for #1008

@zbysir zbysir changed the title Feat: ScrollScreenshot, for https://github.com/go-rod/rod/issues/1008 #1009 Feat: ScrollScreenshot Feb 2, 2024
lib/examples/compare-chromedp/scroll_screenshot/main.go Outdated Show resolved Hide resolved
page.go Show resolved Hide resolved
@ysmood
Copy link
Member

ysmood commented Feb 2, 2024

Good job! It will be great if you could add some tests.

@zbysir
Copy link
Contributor Author

zbysir commented Feb 2, 2024

Good job! It will be great if you could add some tests.

Review again, please allow me to make some weird mistakes.

Copy link
Member

@ysmood ysmood left a comment

Choose a reason for hiding this comment

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

Please fix the lint error in CI, you can check it on your local first go run ./lib/utils/lint.

The test doesn't work on Mac and Windows: https://github.com/go-rod/rod/actions/runs/7751056459/job/21138412821?pr=1010#step:4:25

@zbysir
Copy link
Contributor Author

zbysir commented Feb 2, 2024

Please fix the lint error in CI, you can check it on your local first go run ./lib/utils/lint.

The test doesn't work on Mac and Windows: https://github.com/go-rod/rod/actions/runs/7751056459/job/21138412821?pr=1010#step:4:25

Yes, I fixed it. Done.

@zbysir zbysir requested a review from ysmood February 2, 2024 04:32
@ysmood
Copy link
Member

ysmood commented Feb 2, 2024

Please check the CI, if you need help let me know.

@zbysir
Copy link
Contributor Author

zbysir commented Feb 2, 2024

@ysmood Need help:

The test code cannot cover the error branch below

              err = p.WaitStable(opt.WaitPreScroll)
                if err != nil {
                        return nil, fmt.Errorf("waitStable error: %w", err)
                }

I don't know how to fix it, I tried the test code below but it doesn't work

	g.Panic(func() {
		// mock error for WaitStable
		g.mc.stubErr(1, proto.DOMSnapshotCaptureSnapshot{})
		p.MustScrollScreenshotPage()
	})

Do you have any plans? Or is it appropriate for me to use time.Sleep() directly?

Copy link
Contributor Author

@zbysir zbysir left a comment

Choose a reason for hiding this comment

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

go run ./lib/utils/lint will end normally now

zbysir

This comment was marked as duplicate.

zbysir

This comment was marked as resolved.

@zbysir
Copy link
Contributor Author

zbysir commented Feb 3, 2024

@ysmood Please run CI again and it will almost succeed.

@ysmood ysmood enabled auto-merge (squash) February 3, 2024 08:10
Copy link
Member

@ysmood ysmood left a comment

Choose a reason for hiding this comment

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

👍🏼

@ysmood ysmood merged commit 2042134 into go-rod:main Feb 3, 2024
6 checks passed
@zbysir zbysir deleted the dev branch February 4, 2024 01:57
@zbysir
Copy link
Contributor Author

zbysir commented Feb 4, 2024

@ysmood Thank you for your patient help!

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