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

FileField: show image thumbnails [WiP] #3397

Closed
wants to merge 1 commit into from

Conversation

wmertens
Copy link
Contributor

Make filefields look like this if they are an image:

image

Still to do:

  • proper styling, instead of the inline flex. How should I do this?
  • allow disabling thumbnails, how about with plain: true?

@mxstbr
Copy link
Collaborator

mxstbr commented Aug 24, 2016

How about using the same styling/component the CloudinaryImage field uses?

@wmertens
Copy link
Contributor Author

Sounds good, but that code seems a bit involved. Could you outline the changes needed?

@mxstbr
Copy link
Collaborator

mxstbr commented Aug 24, 2016

No idea, I didn't write it 😀 you'll have to dig in!

@JedWatson
Copy link
Member

👍 good feature, @wmertens

IMO this should be opt-in, because unlike images from cloudinary which we can dynamically resize, general images may be really large (i.e multiple megabytes) which I don't think we should be blindly foisting on users by default.

How about thumbnail: true as the field option to turn this on?

Also, it might be worth looking at something like sharp for thumbnail processing in Keystone. There's also a neat image-resizer project that's sort of like a DIY cloudinary, but it's nontrivial in size and to implement so I don't want to bundle it with Keystone by default.

Then again, we're getting into Image field territory here.

const FileDom = ({ url, filename }) => {
return (
<div style={{ display: 'flex' }}>
<FileThumb {...{ url }}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. why are you sending an object containing url when only url is interesting to the FileThumb component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adnasa This is just React, it's exactly the same as saying <FileThumb url={url}/> and gets transpiled to the exact same React.createElement call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth changing that to <FileThumb url={url} /> – I wasn't 100% sure what that did either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping that JSX will allow writing <FileThumb {url}/>, see facebook/jsx#23

Would that have been less confusing? If so, some upvotes would be nice :)

Copy link
Contributor

Choose a reason for hiding this comment

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

since it is a single prop

<Component singleProp={value} />

is much more readable than

<Component {...{ singleProp } />

@SharadKumar
Copy link

This middleware might help replace most of Cloudinary stuff and work well with S3!
https://www.npmjs.com/package/image-steam

Sharad

On 31 Aug 2016, at 5:27 PM, Jed Watson <[email protected]mailto:[email protected]> wrote:

? good feature, @wmertenshttps://github.com/wmertens

IMO this should be opt-in, because unlike images from cloudinary which we can dynamically resize, general images may be really large (i.e multiple megabytes) which I don't think we should be blindly foisting on users by default.

How about thumbnail: true as the field option to turn this on?

Also, it might be worth looking at something like sharphttps://github.com/lovell/sharp for thumbnail processing in Keystone. There's also a neat image-resizerhttps://github.com/jimmynicol/image-resizer project that's sort of like a DIY cloudinary, but it's nontrivial in size and to implement so I don't want to bundle it with Keystone by default.

Then again, we're getting into Image field territory here.

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com/keystonejs/keystone/pull/3397#issuecomment-243681167, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFO-EMMvW1vJlngGYP8pOUr7qwrRMlWBks5qlSzagaJpZM4Jr5LR.

@wmertens
Copy link
Contributor Author

wmertens commented Sep 5, 2016

@SharadKumar I think that in order to implement this properly we will need asilvas/node-image-steam#39 but I haven't thought it all through yet.

@bassjacob
Copy link
Contributor

Hi @wmertens, thanks for the work on this PR so far. The scope of this seems to have blown out a little bit in the comments, but I think it does need to be styled to get merged. Are you still interested in getting this in?

@wmertens
Copy link
Contributor Author

wmertens commented Dec 22, 2016 via email

@htor
Copy link

htor commented Feb 23, 2017

This should really be merged...

@JedWatson
Copy link
Member

@htor we'd love to have this feature but it needs proper styling before it can be merged, and a way for developers to opt in to it (or at least more thought around the logic that enables it). There are a number of scenarios where blindly rendering a preview of a file based on the file type could be bad UX; key being files can be quite large images, so on mobile or a slow connection simply navigating to the item details screen could kick off a huge download (e.g. original photos)

@outluch
Copy link

outluch commented Apr 8, 2017

Come on, it is so essential feature to show local images in admin UI...

@wmertens
Copy link
Contributor Author

wmertens commented Apr 9, 2017

@outlunch feel free to implement this then :-D

@outluch
Copy link

outluch commented Apr 9, 2017

@wmertens, Already, but I'm not familiar with github pullrequests, etc. Maybe someone could make it correct way?
Here is my bad pull-request:
keystonejs/keystone#4198

@htor
Copy link

htor commented Apr 22, 2018

@wmertens
this can now be closed as image thumbnails are now implemented in keystonejs/keystone#4509

@Pyav123
Copy link

Pyav123 commented Apr 25, 2018

@htor , I am using [email protected] but it is not showing image thumbnail using the local storage, I am using
image:{
type: Types.File,
storage: newStorage,
thumb: true
}
Can someone please tell what else I have to do? I need this feature.

@wmertens wmertens closed this Apr 25, 2018
@htor
Copy link

htor commented Apr 25, 2018

@Pyav123
at the moment you need to depend on the git version of keystone (don't think npm is updated yet), so put this in your package.json and run npm install:

"keystone": "https://github.com/keystonejs/keystone.git"

then see what happens.

@Pyav123
Copy link

Pyav123 commented Apr 26, 2018

Hello @htor I saw your suggestion on https://stackoverflow.com/questions/40879375/keystonejs-4-0-file-system-storage-adapter-image-preview but this is also not working for me, I am getting the code implemented for this inside keystone but unable to see the preview. Please suggest on this.

@Pyav123
Copy link

Pyav123 commented Apr 27, 2018

Hey @htor it is working now , forgot to mention url: true inside the schema , just thumb: true didn't work for me , Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants