storycap icon indicating copy to clipboard operation
storycap copied to clipboard

Feature Request: Take screenshot of a portion of the page

Open sophiabits opened this issue 3 years ago • 3 comments

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 {

sophiabits avatar Jan 07 '22 00:01 sophiabits

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 avatar Apr 12 '22 22:04 Jamie5

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

Jamie5 avatar Jun 02 '22 18:06 Jamie5

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)

Jamie5 avatar Jul 21 '22 00:07 Jamie5

This would neatly solve https://github.com/reg-viz/storycap/issues/186.

emlai avatar Oct 21 '22 16:10 emlai