Skip to content
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

Image field type #5396

Merged
merged 91 commits into from
Apr 16, 2021
Merged

Image field type #5396

merged 91 commits into from
Apr 16, 2021

Conversation

rohan-deshpande
Copy link
Contributor

Added

  • Image field type and API for handling image I/O
  • Image config to Keystone config
  • Required packages for image I/O processes

Notes

  • This is completely untested and may or may not work. I want to get into writing tests for it but would like someone's eyes across it first who is maybe a bit more familiar with the sharp API.
  • Some of the blurhash code is lifted from nodejs implementation woltapp/blurhash#43 (comment) there's maybe some stuff they are doing here that we don't need to do... I'm not too sure
  • If the REST endpoint is also going to use getDataFromStream then it probably makes sense for the file to get written to the storage layer within this function. If that's the case, then we should change the name of the function to something like writeStreamToFile
  • I'm really, really not a fan of the upload key in the config. While I am also not a fan of mode as it is just super generic, I feel it's easier to document than upload so I would be in favour of changing this key to mode. We at least wouldn't have to do something like this since we are calling it mode in every other instance
const { upload: mode } = config;

@vercel
Copy link

vercel bot commented Apr 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/EAeRVwNp9Cs7CBkGCM6nDQWAJtwK
✅ Preview: https://keystone-next-docs-git-image-field-type-keystonejs.vercel.app

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2021

🦋 Changeset detected

Latest commit: ec3a1e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@keystone-next/admin-ui Minor
@keystone-next/app-basic Patch
@keystone-next/fields Minor
@keystone-next/types Minor
@keystone-next/keystone Minor
@keystone-next/auth Major
@keystone-next/test-utils-legacy Patch
@keystone-next/example-auth Patch
@keystone-next/example-ecommerce Patch
keystone-next-app Patch
@keystone-next/example-roles Patch
@keystone-next/example-sandbox Patch
@keystone-next/example-todo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 7, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ec3a1e2:

Sandbox Source
@keystone-next/example-sandbox Configuration

@emmatown
Copy link
Member

emmatown commented Apr 8, 2021

Given we only need to get the type and size of the iamge, could we replace sharp with https://www.npmjs.com/package/file-type + https://www.npmjs.com/package/image-size? There is one problem with that which is svg support which kinda makes things weird but we might just want to avoid supporting svg for now given it's not a binary format so it's kinda totally different to the others and doesn't(at least in my understanding) have a defined width and height in the way the other formats we have do. I'd like to avoid a dependency on a native module like sharp in keystone if we can avoid it.

@vercel vercel bot temporarily deployed to Preview April 9, 2021 00:26 Inactive
@vercel vercel bot temporarily deployed to Preview April 9, 2021 01:22 Inactive
@vercel vercel bot temporarily deployed to Preview April 9, 2021 01:43 Inactive
@vercel vercel bot temporarily deployed to Preview April 9, 2021 02:17 Inactive
@vercel vercel bot temporarily deployed to Preview April 12, 2021 00:06 Inactive
@vercel vercel bot temporarily deployed to Preview April 12, 2021 02:12 Inactive
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Can you(@rohan-deshpande and/or @gwyneplaine) resolve the conflicts, add changesets, convert the Implementation to TypeScript like the other fields now are and originally the plan was to store the data in seperate columns rather than JSON, do we want to do that now?

packages-next/keystone/package.json Outdated Show resolved Hide resolved
packages-next/keystone/package.json Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview April 16, 2021 03:36 Inactive
packages/utils/src/index.ts Outdated Show resolved Hide resolved
packages/utils/src/index.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview April 16, 2021 03:51 Inactive
@gwyneplaine gwyneplaine requested a review from emmatown April 16, 2021 03:54
@vercel vercel bot temporarily deployed to Preview April 16, 2021 04:14 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2021 04:17 Inactive
@gwyneplaine gwyneplaine requested a review from emmatown April 16, 2021 04:20
@vercel vercel bot temporarily deployed to Preview April 16, 2021 04:22 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2021 04:24 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2021 04:30 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2021 05:07 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2021 05:15 Inactive
@emmatown emmatown enabled auto-merge (squash) April 16, 2021 05:17
@emmatown emmatown merged commit be60812 into master Apr 16, 2021
@emmatown emmatown deleted the image-field-type branch April 16, 2021 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants