[GitHub] [cordova-plugin-camera] breautek commented on a change in pull request #435: Update browser plugin

[GitBox <gi...@apache.org>]

breautek commented on a change in pull request #435: Update browser plugin
URL: https://github.com/apache/cordova-plugin-camera/pull/435#discussion_r338153950
 
 

 ##########
 File path: README.md
 ##########
 @@ -266,6 +266,10 @@ Optional parameters to customize the camera settings.
 | saveToPhotoAlbum | <code>Boolean</code> |  | Save the image to the photo album on the device after capture. |
 | popoverOptions | <code>[CameraPopoverOptions](#module_CameraPopoverOptions)</code> |  | iOS-only options that specify popover location in iPad. |
 | cameraDirection | <code>[Direction](#module_Camera.Direction)</code> | <code>BACK</code> | Choose the camera to use (front- or back-facing). |
+| customCameraContainer | <code>string</code> |  | Browser-only option, specify the id of custom camera's container element.  |
 
 Review comment:
   `customCameraContainer` sounds like is something you need a specific `DOMElement`, so allowing flexibility of `querySelector` I think promotes bad coding practice.
   
   If you need a specific element, the element should be tagged by an ID as to guarantee that you will receive the expected `DOMElement`. Using `querySelector` makes code brittle by picking the first occurrence of the selected term, which may not be the node you want.
   
   The YouTube JS API[1] for example does something similar where it needs to insert HTML in a specific node of your choice. Their API signature accepts either an id, or the `HTMLElement` itself.
   
   So I would prefer to keep `customCameraContainer` accepting an `id` rather than a query string. For flexibility, it could also optionally accept a DOM node and it would be up to the developer to find that reference as they see fit (which they could use `querySelector` if they wanted to, or it could be a fresh node that isn't even in the DOM tree yet (via `document.createElement`), etc.
   
   [1] https://developers.google.com/youtube/iframe_api_reference#Loading_a_Video_Player

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org