-
Notifications
You must be signed in to change notification settings - Fork 51
Add Camera API type definitions #3597
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
base: 2026-01-rc
Are you sure you want to change the base?
Conversation
4798f27 to
808eb9f
Compare
|
/snapit |
|
Test the snapshot by updating your "@shopify/ui-extensions": "0.0.0-snapshot-20251125223311" |
packages/ui-extensions/src/surfaces/point-of-sale/api/action-target-api/action-target-api.ts
Show resolved
Hide resolved
packages/ui-extensions/src/surfaces/point-of-sale/api/camera-api/camera-api.ts
Outdated
Show resolved
Hide resolved
packages/ui-extensions/src/surfaces/point-of-sale/api/camera-api/camera-api.ts
Outdated
Show resolved
Hide resolved
packages/ui-extensions/src/surfaces/point-of-sale/api/camera-api/camera-api.ts
Outdated
Show resolved
Hide resolved
packages/ui-extensions/src/surfaces/point-of-sale/api/camera-api/camera-api.ts
Show resolved
Hide resolved
808eb9f to
d971e35
Compare
packages/ui-extensions/src/surfaces/point-of-sale/api/camera-api/camera-api.ts
Outdated
Show resolved
Hide resolved
vctrchu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just the one comment about removing the POS GO reference
d971e35 to
94bf96c
Compare
94bf96c to
4cf982d
Compare
| */ | ||
| maxHeight?: number; | ||
| /** | ||
| * The quality of the image returned. Percentile value between 0 and 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 0-1 based on some standards or is it made up for this?
Piggybacking off what Victor was asking I think we need to give some more context of that 0 and what 1 gives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0-1 range (percentile/percentage) is common way to represent compression quality levels. we can add (0 = lowest quality, 1 = highest quality) here so it's clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the standard for image compression APIs on Native surfaces like IOS (see here)
| /** The height of the image in pixels. */ | ||
| height: number; | ||
| /** The file size of the image in bytes. */ | ||
| fileSize: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this fileSize would change depending on the quality number correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, as well as image dimensions.
| /** The file size of the image in bytes. */ | ||
| fileSize: number; | ||
| /** The MIME type of the image. */ | ||
| type: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have this be an enum if it is MIME type?
And what is the value of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Value for type here would be, image/jpeg, image/png, etc.
Using string is preferable to a hardcoded enum since MIME types can evolve and the OS/libraries may support additional types without us needing to update an enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we are testing all use cases I think this is not a great idea.
It is easier to add types than to try to remove cases that don't work later.
We have a similar MIME type on embed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the MIME type value for the response and since the device took the photo, it'll always be the correct MIME value. maintaining enum would be toil here.
4cf982d to
101406b
Compare
Co-authored-by: Han T. <[email protected]>
101406b to
4603535
Compare
Closes #21032
Background
Add type definitions for Camera API
Checklist