Skip to content
This repository has been archived by the owner on Apr 25, 2024. It is now read-only.

Changed masonry grid to flex grid #538

Merged
merged 9 commits into from
Feb 18, 2020
Merged

Changed masonry grid to flex grid #538

merged 9 commits into from
Feb 18, 2020

Conversation

dos077
Copy link
Contributor

@dos077 dos077 commented Nov 25, 2019

Fixes #489

[Short description explaining the high-level reason for the pull request]

Dropped masonry grid and changed the image grid to pure css flexbox sizing grid. Based on this blog post: xieranmaya/blog#6

Added aspect ratio restrictions to maintain thumbnail readability, maximum of 16:9 and minimum of 3:4. Thumbnails will be centered and crop if it exceeds aspect ratios. Panaromas(> 21:9) will use its original image as thumbnails instead of highly compressed thumbnails.

[If this Pull Request contains any UI changes, provide an image or GIF displaying the changes]

https://drive.google.com/file/d/1t1aHJxxydBjgCiVFkl1jswWjeiPHCV6V/view?usp=sharing

https://drive.google.com/file/d/1c8n5B5g1jEnArOZx7-gmrqwvByv3Tz6z/view?usp=sharing


Developer Certificate of Origin Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 1 Letterman Drive Suite D4700 San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Copy link
Contributor

@brenoferreira brenoferreira left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you.
Just a minor comment:

The source logo looks a bit strange. Could you take a look and fix it please?

Screen Shot 2019-11-29 at 16 27 25
it looks a bit stretched out

Screen Shot 2019-11-29 at 16 27 36
this is what it looks like before your change

@@ -1,10 +1,14 @@
<template>
<div>
<div class="search-grid_item-container"
:style="`width: ${containerAspect * 165}px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this 165 value to a constant, with an appropriate name please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to a data value that's calculated with min row width / max aspect, so min img container width will not exceed min row width.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the provider logo aspect ratio seems to be a problem with the original css? it has a 40 to 30 max dimension even though the logo is a square. Changing the dimension limit to 30 by 30 seems to solve the problem.

src/components/SearchGridCell.vue Outdated Show resolved Hide resolved
@brenoferreira
Copy link
Contributor

As far as I can see, this looks good.
There's only one caveat: for some data sources, the API doesn't send the width and height attributes, so they don't render at all, for example, if you try on your machine https://ccsearch-dev.creativecommons.org/collections/met, you'll see that the image grid doesn't appear on screen.

I filed a bug on the API repo https://github.com/creativecommons/cccatalog-api/issues/397 and will wait for the feedback there. I'll keep this PR open in the meantime so we can try it once the problem is fixed

@dos077
Copy link
Contributor Author

dos077 commented Dec 6, 2019

I was thinking about this over the weekend while trying to turn this grid into a generic component. I added a get dimension function in case the image.width /.height props are not set. This should work as long as the thumbnail renders.

Copy link
Contributor

@brenoferreira brenoferreira left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I made just one small change to make the images look a bit bigger.

Thank you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Drop Masonry Layout for image grid
3 participants