Extend screenshot with specific file path in local & element by selector #137

elisherer · 2017-08-02T08:54:32Z

I extended the screenshot() function to receive 2 optional arguments

selector (much like the other functions) - (optionally waits and) captures the viewport of an element
options - for future extend-ability an object of options; starting with filePath in case the consumer wants to save the image in a specific path. (use case would be that running user will not have write permissions to the OS temp folder)

Also, fixed some typos of chromlessOptions to chromelessOptions (chrome missing e)

…fic element (by selector)

joelgriffith · 2017-08-02T15:26:27Z

src/chrome/local-runtime.ts

@@ -244,8 +246,20 @@ export default class LocalRuntime {
  }

  // Returns the S3 url or local file path
-  async returnScreenshot(): Promise<string> {
-    const data = await screenshot(this.client)
+  async returnScreenshot(selector?: string, options?: ScreenshotOptions): Promise<string> {


What if consumers want to pass options but not a selector?

You are right, I added the omit check to the api

…ocal-screenshot # Conflicts: # README.md # docs/api.md # src/api.ts # src/chrome/local-runtime.ts # src/types.ts # src/util.ts

…ttier

… feature/extend-local-screenshot

# Conflicts: # src/api.ts # src/chrome/local-runtime.ts # src/util.ts

adieuadieu

Hi @elisherer thank you for this PR. It's great that you've been able to address more or less all of #113! I left a few comments on the code of some issues I noticed. Did you test out the screenshot('.myDiv') functionality and confirm it works as expected?

adieuadieu · 2017-08-06T10:25:20Z

src/util.ts

+  }
+
+  const screenshot = await Page.captureScreenshot({
+    format: 'png',


Shouldn't we be passing captureScreenshotOptions here?

lost in an inner merge, sorry.
will add a unit test, so it will not happen,

adieuadieu · 2017-08-06T10:34:37Z

src/util.ts

+  }
+}
+
+function writeToFileSync(data) {


What's this?

it's a mistake, obviously 😀

adieuadieu · 2017-08-06T10:37:41Z

src/util.ts

+  return process.env['CHROMELESS_S3_BUCKET_NAME']
+}
+
+function getS3BucketURL() {


It's nit-picky, but I'd prefer we call this getS3BucketUrl() as it's more consistent with the naming convention throughout the rest of the code.

…erver, bug fix for pdf write file, safer evaluation of expressions by string because tsc adds unwanted code

… on running chrome's version.

# Conflicts: # package.json # src/api.ts # src/chrome/local-runtime.ts # src/util.test.ts

elisherer · 2017-08-06T19:46:49Z

Would appreciate any help with running the unit tests, It works very well on my machine. I wanted to stop using Google and start using our own page in chrome.

joelgriffith · 2017-08-07T15:57:36Z

src/chrome/local-runtime.ts

+    if (isS3Configured()) {
+      return await uploadToS3(data, 'image/png')
+    } else {
+      return writeToFile(data, 'png', options && options.filePath)


Is filePath truly required? Otherwise this might pass an object to writeToFile

either options is falsy or it is truthy and then it would be the value of filePath (or I didn't understand your concern?)

joelgriffith · 2017-08-07T16:01:49Z

src/util.ts

@@ -56,9 +59,9 @@ export async function waitForNode(
  waitTimeout: number,
 ): Promise<void> {
  const { Runtime } = client


Curious why you stringified these functions... I'm pretty sure they get stringified downstream. Is that not the case?

when running ava under nyc, it is added with coverage code which breaks once passed to the browser (i.e cov_### globals)

… feature/extend-local-screenshot

elisherer · 2017-08-13T09:42:39Z

@adieuadieu, have you got the chance to go over my changes yet?
@joelgriffith, have you seen my responses to your comments?

adieuadieu

Hi @elisherer thank you for your continued effort on this PR and addressing our feedback. I apologize for the delay from our end in reviewing it.

Using our own page for the tests rather than google.com is definitely the right direction.

If you could resolve the merge conflict introduced by #224 (CHROMELESS_S3_OBJECT_KEY_PREFIX env variable), then I'd be ready to approve the PR. 😃

adieuadieu · 2017-08-13T15:39:40Z

src/util.test.ts

+const testHtml = fs.readFileSync('./src/__tests__/test.html')
+const testUrl = `data:text/html,${testHtml}`
+
+const getPngMetaData = async (filePath): Promise<any> => {


Great job not introducing a dependency!

… feature/extend-local-screenshot # Conflicts: # src/chrome/local-runtime.ts

elisherer · 2017-08-15T07:57:34Z

@adieuadieu, no more conflicts 😄

…-screenshot Extend screenshot with specific file path in local & element by selector

Eli Sherer added 2 commits August 2, 2017 11:45

extend screenshot for specific file path in local and clip on a speci…

de9520c

…fic element (by selector)

screenshot - add implicit wait and node existence check

640bb18

This was referenced Aug 2, 2017

Abstract the shared file-saving code between .screenshot() and .pdf() #113

Closed

Take screenshot of an element #41

Closed

Modify path of screenshot/pdf #83

Closed

joelgriffith reviewed Aug 2, 2017

View reviewed changes

elisherer added 3 commits August 2, 2017 22:47

make it possible to omit selector, fix tslint warnings

3b30978

Merge remote-tracking branch 'graphcool/master' into feature/extend-l…

5d4a5f4

…ocal-screenshot # Conflicts: # README.md # docs/api.md # src/api.ts # src/chrome/local-runtime.ts # src/types.ts # src/util.ts

add example of omitting selector in screenshot inside readme, and pre…

7c4e0a7

…ttier

adieuadieu added API enhancement labels Aug 2, 2017

Eli Sherer added 2 commits August 3, 2017 13:20

add filePath to pdf also to solve schickling#129 and schickling#113

c534182

Merge branch 'master' of https://github.com/graphcool/chromeless into…

1d1db4f

… feature/extend-local-screenshot

adieuadieu requested review from timsuchanek, joelgriffith and schickling August 3, 2017 19:47

adieuadieu added this to the v1.2 milestone Aug 3, 2017

This was referenced Aug 4, 2017

Fix: Generating screenshots/pdf using os.tmpdir() for Windows #172

Merged

Screenshot fails on Windows, if there's no c:\tmp folder #129

Closed

elisherer added 2 commits August 5, 2017 00:04

Merge branch 'master' into feature/extend-local-screenshot

58a8109

# Conflicts: # src/api.ts # src/chrome/local-runtime.ts # src/util.ts

move all fs and s3 handling to util to handle schickling#113

3074c6b

adieuadieu suggested changes Aug 6, 2017

View reviewed changes

adieuadieu modified the milestones: v1.3, v1.2 Aug 6, 2017

Eli Sherer and others added 4 commits August 6, 2017 18:25

code review remarks fixed, better unit tests using a dedicated http s…

d0f6002

…erver, bug fix for pdf write file, safer evaluation of expressions by string because tsc adds unwanted code

change use of promisify to using actual Promise, fix test to be based…

3274356

… on running chrome's version.

Merge branch 'master' into feature/extend-local-screenshot

9babd26

# Conflicts: # package.json # src/api.ts # src/chrome/local-runtime.ts # src/util.test.ts

fix failing test server (legacy node), fix unit tests for windows

bba39fe

elisherer added 3 commits August 6, 2017 23:03

revert running a test server and use a static html file for testing

b0cb18e

apparently screenshot clip works since Chrome 61 and not 62

d1540e2

fix unit test version detection

005cf14

elisherer mentioned this pull request Aug 7, 2017

Add "setUserAgent" method to example #194

Open

joelgriffith reviewed Aug 7, 2017

View reviewed changes

Merge branch 'master' of https://github.com/graphcool/chromeless into…

abfe205

… feature/extend-local-screenshot

adieuadieu mentioned this pull request Aug 12, 2017

how to set the screenshot path when run locally #226

Closed

adieuadieu suggested changes Aug 13, 2017

View reviewed changes

adieuadieu requested a review from joelgriffith August 13, 2017 15:47

Merge branch 'master' of https://github.com/graphcool/chromeless into…

03bb339

… feature/extend-local-screenshot # Conflicts: # src/chrome/local-runtime.ts

adieuadieu modified the milestones: v1.4, v1.3 Aug 29, 2017

adieuadieu approved these changes Aug 30, 2017

View reviewed changes

Merge branch 'master' into feature/extend-local-screenshot

85391e3

adieuadieu merged commit af5f380 into schickling:master Aug 30, 2017

adieuadieu added a commit that referenced this pull request Aug 30, 2017

doc: add #137 to changelog

a039588

adieuadieu mentioned this pull request Aug 30, 2017

returnPdf key prefix #249

Closed

elisherer deleted the feature/extend-local-screenshot branch August 30, 2017 12:46

webpolis pushed a commit to webpolis/yb-chromeless that referenced this pull request Oct 5, 2017

Merge pull request schickling#137 from elisherer/feature/extend-local…

f5a2e4b

…-screenshot Extend screenshot with specific file path in local & element by selector

webpolis pushed a commit to webpolis/yb-chromeless that referenced this pull request Oct 5, 2017

doc: add schickling#137 to changelog

532eba7

mattcanty mentioned this pull request Feb 13, 2018

Full page screenshot #92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend screenshot with specific file path in local & element by selector #137

Extend screenshot with specific file path in local & element by selector #137

elisherer commented Aug 2, 2017

joelgriffith Aug 2, 2017

elisherer Aug 2, 2017

adieuadieu left a comment •

edited

Loading

adieuadieu Aug 6, 2017

elisherer Aug 6, 2017 •

edited

Loading

adieuadieu Aug 6, 2017

elisherer Aug 6, 2017

adieuadieu Aug 6, 2017

elisherer Aug 6, 2017

elisherer commented Aug 6, 2017

joelgriffith Aug 7, 2017

elisherer Aug 7, 2017

joelgriffith Aug 7, 2017

elisherer Aug 7, 2017

elisherer commented Aug 13, 2017

adieuadieu left a comment

adieuadieu Aug 13, 2017

elisherer commented Aug 15, 2017

Extend screenshot with specific file path in local & element by selector #137

Extend screenshot with specific file path in local & element by selector #137

Conversation

elisherer commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adieuadieu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer commented Aug 13, 2017

adieuadieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer commented Aug 15, 2017

adieuadieu left a comment •

edited

Loading

elisherer Aug 6, 2017 •

edited

Loading