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

Feature Request: Take screenshot of a portion of the page #506

Closed
sophiabits opened this issue Jan 7, 2022 · 4 comments · Fixed by #722
Closed

Feature Request: Take screenshot of a portion of the page #506

sophiabits opened this issue Jan 7, 2022 · 4 comments · Fixed by #722

Comments

@sophiabits
Copy link

Thanks for making such a great library available for everyone to use. At our company we're getting a lot of use out of it :)

One feature we need internally is to take a screenshot of a portion of particular stories. Specifically, we render out dom nodes with a data-canvas-strip attribute and we're interested in ensuring that the screenshot only contains that node and nothing else. We don't necessarily know the height of each story ahead of time (it varies between stories and hardcoding is isn't portable across different viewports) so simply cropping the screenshots after taking them isn't feasible.

I've attached a proof of concept diff which adds a selector field which takes a CSS selector to the ScreenshotOptions interface and it works great for us, but we'd like to upstream it if possible so that we don't need to maintain a fork and so that other people can use the feature if they think it's useful.

I'm more than happy to take the diff and improve it so that it can be PRed. I would just need some pointers on the following:

  • Is my general approach correct? I've added a captureRawScreenshotBuffer method to the CapturingBrowser class which handles the selector option. This was the first place I saw I could add it, so I'm not sure if there's a more appropriate location for it.
  • If the user provides a selector that doesn't match an element, should that be an error or should we just fall back to capturing the entire viewport? We don't hit this case in our code because we always have a valid selector but of course it could happen to someone using the package.
  • Is there any need for test cases? The only test I can see is the e2e.sh file which runs storycap and asserts that at least one screenshot file is emitted. If that's the only test currently then I assume this feature doesn't need any additional tests?

Please let me know your thoughts on this and I'll get to work on a proper PR if you're happy to accept one for this feature.


Current patch:

diff --git a/packages/storycap/src/node/capturing-browser.ts b/packages/storycap/src/node/capturing-browser.ts
index 64c49ee..ffea89a 100644
--- a/packages/storycap/src/node/capturing-browser.ts
+++ b/packages/storycap/src/node/capturing-browser.ts
@@ -307,6 +307,28 @@ export class CapturingBrowser extends StoryPreviewBrowser {
     }
   }
 
+  private async captureRawScreenshotBuffer(screenshotOptions: ScreenshotOptions) {
+    if (screenshotOptions.selector) {
+      await this.page.waitForSelector(screenshotOptions.selector);
+      const element = await this.page.$(screenshotOptions.selector);
+
+      if (!element) {
+        // TODO: What to do?
+        throw new Error();
+      }
+
+      return element.screenshot({
+        fullPage: screenshotOptions.fullPage,
+        omitBackground: screenshotOptions.omitBackground,
+      });
+    } else {
+      return this.page.screenshot({
+        fullPage: screenshotOptions.fullPage,
+        omitBackground: screenshotOptions.omitBackground,
+      });
+    }
+  }
+
   private logInvalidVariantKeysReason(reason: InvalidVariantKeysReason | null) {
     if (reason) {
       if (reason.type === 'notFound') {
@@ -399,10 +421,7 @@ export class CapturingBrowser extends StoryPreviewBrowser {
     await this.page.evaluate(() => new Promise(res => (window as any).requestIdleCallback(res, { timeout: 3000 })));
 
     // Get PNG image buffer
-    const rawBuffer = await this.page.screenshot({
-      fullPage: emittedScreenshotOptions.fullPage,
-      omitBackground: emittedScreenshotOptions.omitBackground,
-    });
+    const rawBuffer = await this.captureRawScreenshotBuffer(emittedScreenshotOptions);
 
     let buffer: Buffer | null = null;
     if (Buffer.isBuffer(rawBuffer)) {
diff --git a/packages/storycap/src/shared/screenshot-options-helper.ts b/packages/storycap/src/shared/screenshot-options-helper.ts
index 3868b0a..c2d4e6e 100644
--- a/packages/storycap/src/shared/screenshot-options-helper.ts
+++ b/packages/storycap/src/shared/screenshot-options-helper.ts
@@ -69,6 +69,7 @@ export function createBaseScreenshotOptions({
       viewport: viewports[0],
       variants: viewports.slice(1).reduce((acc, vp) => ({ ...acc, [vp]: { viewport: vp } }), {}),
       defaultVariantSuffix: viewports[0],
+      selector: '',
     };
   } else {
     return {
@@ -77,6 +78,7 @@ export function createBaseScreenshotOptions({
       waitAssets: !disableWaitAssets,
       viewport: viewports[0],
       defaultVariantSuffix: '',
+      selector: '',
     };
   }
 }
diff --git a/packages/storycap/src/shared/types.ts b/packages/storycap/src/shared/types.ts
index cd15abe..1690b9a 100644
--- a/packages/storycap/src/shared/types.ts
+++ b/packages/storycap/src/shared/types.ts
@@ -23,6 +23,7 @@ export interface ScreenshotOptionFragments {
   click?: string;
   skip?: boolean;
   omitBackground?: boolean;
+  selector?: string;
 }
 
 export interface ScreenshotOptionFragmentsForVariant extends ScreenshotOptionFragments {
@Jamie5
Copy link
Contributor

Jamie5 commented Apr 12, 2022

Might also be nice to be able to pass clip directly or if it's possible, pass an element directly instead of via selector.

@Jamie5
Copy link
Contributor

Jamie5 commented Jun 2, 2022

@Quramy would this be something that might happen soon or would you advise forking and doing similar to above?

@Jamie5
Copy link
Contributor

Jamie5 commented Jul 21, 2022

https://github.com/remix/storycap/pull/3/files is a similar way to do things, in which we manually specify the bounding box (which may have its uses, especially when the actual screenshot-taking code is wrapped in one's own layer anyways)

@emlai
Copy link

emlai commented Oct 21, 2022

This would neatly solve #186.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants