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

Add "Crop image previews" setting to files #25055

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Add "Crop image previews" setting to files #25055

merged 1 commit into from
Jan 15, 2021

Conversation

nina-py
Copy link
Contributor

@nina-py nina-py commented Jan 10, 2021

Added a new user setting that toggles cropping on image previews in grid view.

True (default value): crops each image to a square. False: keep original aspect ratio.

Signed-off-by: Nina Pypchenko [email protected]

Closes #18439.

@nina-py
Copy link
Contributor Author

nina-py commented Jan 10, 2021

It's funny that the "Node / build" check is failing because I did check in the compiled files and running make build-js-production produces nothing. Could the bot help? (Did a soft reset, restored the /dist/ files, generated them again, committed everything and force-pushed the commit and it's still failing.)

Also, please note that I added a test on the backend for the new setting but I haven't found a way to add a working test for the frontend code. If I add a test in filelistSpec.js with the following:

var image = new FileInfo({
   id: 123,
   name: 'a-photo.jpeg',
   mimetype: 'image/jpeg',
   size: 1234,
   etag: 'a01234c',
   mtime: 123456
});
var $tr = fileList.add(image);
console.log($tr.find('.thumbnail').css('background-image'));

...the output I get back is:

url("http://localhost/core/img/filetypes/image.svg")

...which is not very helfpul if I want to test changes to preview image URLs. On closer look I see that the generatePreviewUrl method doesn't seem to have any existing tests. Should there be any? Or is it all going to be scrapped and replaced with Vue code soon so it's not really a worry?

@nina-py
Copy link
Contributor Author

nina-py commented Jan 13, 2021

Hi @kesselb, could I try running that compile command you executed in my first PR, but for apps/files/js to see if helps with the build test? Or do I have to be a member of Nextcloud to be able to do that?

@nina-py
Copy link
Contributor Author

nina-py commented Jan 13, 2021

Here is a GIF of the new setting in action, by the way. It reloads the current file list immediately and sends an API call to save the setting to the database after a 1.2 sec delay.

nextcloud-crop-image-previews-demo

@ChristophWurst
Copy link
Member

I can't explain that build diff tbh but could it be related to the node env? Right now some of a newer node/npm and others have the slightly older one. It could be a mismatch there. IIRC there is also an issue with the latest npm due to missing or incompatible peer deps in our dependency tree.

Run actions/setup-node@v1
/opt/hostedtoolcache/node/12.20.0/x64/bin/node --version
v12.20.0
/opt/hostedtoolcache/node/12.20.0/x64/bin/npm --version
6.14.8

^ that is what the Github action builds with.

@nina-py
Copy link
Contributor Author

nina-py commented Jan 14, 2021

Thank you @ChristophWurst, I thought that might be the case. I have Node 14 installed locally, and my npm version is just a couple of minor updates newer than what the bot is using (6.14.10). I have successfully checked in compiled assets before (#24970), but that was a much smaller chunk of work.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Code looks good! Great work!!

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Reading jquery code makes me cringe but it's a solid PR until we have a Vue rewrite 😜

@nina-py
Copy link
Contributor Author

nina-py commented Jan 14, 2021

Thank you @skjnldsv!

@ChristophWurst, I haven't written any jQuery code for quite a while so it was a bit of time travel for me :). Looking forward to getting into Vue!

@ChristophWurst
Copy link
Member

/compile amend /

Added a new user setting that toggles cropping on image previews
in grid view.

True (default value): crops each image to a square. False: keep original
aspect ratio.

Signed-off-by: Nina Pypchenko <[email protected]>

Closes #18439.

Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 14, 2021
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Very nice 👍

@juliusknorr juliusknorr merged commit c80e007 into nextcloud:master Jan 15, 2021
@rullzer rullzer mentioned this pull request Jan 15, 2021
15 tasks
@nina-py nina-py deleted the 18439-add-crop-image-previews-setting branch January 15, 2021 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to not crop images on grid view
4 participants