Skip to content
This repository has been archived by the owner on Nov 24, 2018. It is now read-only.

feat: selectFile and selectFiles API #170

Merged
merged 4 commits into from
Aug 6, 2017

Conversation

criticalbh
Copy link
Contributor

@criticalbh criticalbh commented Aug 3, 2017

@criticalbh criticalbh mentioned this pull request Aug 3, 2017
Copy link
Collaborator

@adieuadieu adieuadieu left a comment

Choose a reason for hiding this comment

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

Thank you for raising this PR, @criticalbh.

Two points:

  1. We need to think about how this would work over the Proxy. E.g. we might need to add support for uploading from an S3 bucket or other URL. e.g.
await chromeless.selectFile('.uploader', [
  's3://my-bucket/my/object/prefix/file.jpg',
  'http://sweet-site.cool/my-image.jpg'
])
  1. I wonder if we could keep the API methods as just one which can take either a single file path string, or an array of file path strings. Maybe something like .selectFileInput(filePathString | array[filePathString])? (I know we've already asked you to change the name once—sorry!) What do you think @schickling @joelgriffith @timsuchanek ?

@schickling
Copy link
Owner

Sorry for the back and forth on this but I agree with @adieuadieu. We should go with selectFile (or selectFileInput - any other suggestions? @joelgriffith?) and support both a single file and multiple files.

@adieuadieu
Copy link
Collaborator

@schickling I lean towards selectFileInput because it's clearer that it has to do with <input type="file"/> than selectFile. But maybe there's a third name which is even clearer.

@joelgriffith
Copy link
Contributor

I'm ready for a bike-shed! selectFileInput is good, but it strikes me as focusing on the file input rather than uploading a file. Following the conventions of thetype API, we could just call it upload (which takes a string of the input-selector and a single file or an array of files).

@adieuadieu
Copy link
Collaborator

adieuadieu commented Aug 4, 2017

@joelgriffith Ah shoot. You're right. selectFileInput() does sound like you're selecting the actual file-input field. But, I agree with @schickling that upload() is misleading, too. selectInputFile()?

@criticalbh
Copy link
Contributor Author

Why not like CDT api -> setFileInputFiles?

@adieuadieu
Copy link
Collaborator

adieuadieu commented Aug 6, 2017

@joelgriffith @schickling @criticalbh Naming stuff is hard. Since we sort of liked selectFileInput() but didn't like the part that sounded like we were selecting something, shall we take set from CDPs method and go with setFileInput()?

@schickling
Copy link
Owner

I like that proposal. Let's do it!

Copy link
Owner

@schickling schickling left a comment

Choose a reason for hiding this comment

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

Good to go once we've renamed to setFileInput

Copy link
Collaborator

@adieuadieu adieuadieu left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, @criticalbh!

@adieuadieu adieuadieu merged commit 1da78b5 into schickling:master Aug 6, 2017
This was referenced Aug 6, 2017
@criticalbh criticalbh deleted the feature/selectFiles branch August 6, 2017 18:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants