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

Make extending of the last row optional #103

Closed
wants to merge 1 commit into from
Closed

Make extending of the last row optional #103

wants to merge 1 commit into from

Conversation

rzhosan1
Copy link

@rzhosan1 rzhosan1 commented Aug 7, 2018

Hi all!

Images in the last row are extended to fill all the space regardless of the columns property (only single image will keep its size). The last row looks too giant if columns value is more than 3.
https://codesandbox.io/s/0olwwy50n0

So I amended the calculation of images width in the last row in utils.js file.
The default behavior hasn't been changed. But now it's possible to configure this behavior (set min count of images in the last row when it should be extended).
Also I added tests for it, changed styling in several files and changed the first example in order see that problem is solved (2 images don't fill the full size anymore).

This library is awesome, I really like it! Thank you very much!
I'm using it in production and the behavior described above has become a problem for me.
Could you please review this PR and provide your feedback about whether it's possible to commit my changes and publish the new version to npm?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.915% when pulling ac349a3 on RZhosan:master into 4921bdc on neptunian:master.

@neptunian
Copy link
Owner

neptunian commented Aug 11, 2018

Thanks for bringing up the issue. I don't think this would work well in all scenarios so I'm going to extend the existing solution that's currently in place when there is 1 image let in the last row. That is, it will calculate the average ratio from previous rows and use that and not fill up the extra space when the last row has less images than columns. In other words, images should never span across other columns. For example:
screen shot 2018-08-11 at 1 55 59 pm

@neptunian neptunian closed this Aug 11, 2018
@rzhosan1
Copy link
Author

So you've decided to use average height of previous rows for the last row.
Great, it solved my issue, thank you!

@rzhosan1
Copy link
Author

Just noticed an issue when images < columns.

image

@neptunian
Copy link
Owner

neptunian commented Aug 13, 2018

Yes, that will break it. you should see a React proptype warning in your console. Images has to be equal to or greater than columns.

@rzhosan1
Copy link
Author

Yes, see it.
Do you suggest to use columns={Math.min(images.length, columns)} ?

@rzhosan1
Copy link
Author

Btw, you said that my solution wouldn't work well for all scenarios.
Did you mean something like that (different row heights and image sizes)?

image

@neptunian
Copy link
Owner

neptunian commented Aug 13, 2018

It can be equal so if you want all your images on one row you can set columns to equal images.length.

I don't think your solution would be used often. The user would need to update it across their breakpoints or it would break the layout. They also dont know what the last images could be. If its a portrait image you'd probably not want that to stretch across columns or the same amount of columns whereas if it was a panoramic or landscape you would. I think most people would want the images to look in proportion to previous ones.

@rzhosan1
Copy link
Author

Agreed, thanks.

@rzhosan1
Copy link
Author

Btw, would you be able to publish new version to npm?

@neptunian
Copy link
Owner

It was published a couple days ago

@rzhosan1
Copy link
Author

rzhosan1 commented Aug 13, 2018

Unfortunately, can't see any changes so far.
Did I miss something?

(Updated version in package.json to 6.1.4, tested using codesandbox and local project).
https://codesandbox.io/s/0olwwy50n0

@neptunian
Copy link
Owner

Sorry, forgot to build it. Also, I decided to remove the propType for column where it needs to be >= to the length of photos. It will work how it did where if its more, it will just adjust to the amount that exists. Will publish shortly

@rzhosan1
Copy link
Author

Sorry, but there is an issue with adjusting of images.
https://codesandbox.io/s/0olwwy50n0

@neptunian
Copy link
Owner

The columns don't adjust by themselves. Currently you have to dynamically change them yourself at a breakpoint of your choosing: http://neptunian.github.io/react-photo-gallery/examples/dynamic-columns.html

In the next release, I plan to have default columns at default breakpoints.

@rzhosan1
Copy link
Author

Yes, I saw it. But I think it's a different case.
Before the last changes three different Galleries with the same columns value were rendered as:
image
Now they are rendered as
image
So the default behaviour has changed.
As for me user would like to see approximately the same sizes of images regardless the length of images array.

@rzhosan1
Copy link
Author

And that's what I tried to achieve in my PR.
image

@neptunian
Copy link
Owner

ah yes... that is a bug where the single image is too big. i'll look into it.

@rzhosan1
Copy link
Author

There is an example of the same layout with the changes from my PR.
https://codesandbox.io/s/6wqzzm34x3

@neptunian
Copy link
Owner

The 1 image stretched across is acting correctly in the sense that if you have less images than you do columns its going to essentially fit width where columns will equal images. so the default behaviour that i need to implement is that if the user specifies more columns than images, the image should not stretch but leave room as if there were 3 images. In the case where there are more rows to find an average i can use that. In the case where there is only one row, i'll need to create an algorithm for that.

@rzhosan1
Copy link
Author

Don't you like the algorithm I used?
rowWidth / totalRatio * (row.length / columns);

As for me, it makes sense to assume that average ratio will be the same for all row. And it gives the result I'd expect.

@neptunian
Copy link
Owner

neptunian commented Aug 14, 2018

Yes, i like it! I'll release the changes later today. thank you!

@rzhosan1
Copy link
Author

Thank you!
I'm looking forward to test it.

@neptunian
Copy link
Owner

It's up on NPM now.

@rzhosan1
Copy link
Author

Looks good!
Thank you!

@rzhosan1
Copy link
Author

rzhosan1 commented Aug 15, 2018

Default columns with masonry layout look very good!
What do you think, would it be possible to extract columns configuration as separate property,
e.g. columnsConfig={[ { width: 0, columns: 1 }, { width: 500, columns: 2 }, { width: 900, columns: 3 }, { width: 1500, columns: 4 }, ]};

@rzhosan1 rzhosan1 deleted the master branch August 15, 2018 07:14
@OriAmir
Copy link

OriAmir commented Aug 18, 2018

@rzhosan @neptunian from all this discussion I don’t understand what’s the solution for the problem.
Let’s say I set 6 images per row and my total images is 7, so in the second row I have 1 picture that big from the last 6 others , how can I fix that.
Another case - I set 6 per row buy my total is 3 images- what I do about this case ?

Thanks

@rzhosan1
Copy link
Author

Hi @OriAmir .

The solution should already be in the latest version of the library.
Please have a look at https://codesandbox.io/s/6wqzzm34x3.

@OriAmir
Copy link

OriAmir commented Aug 19, 2018

@rzhosan @neptunian I am with the last version 6.2.0 and look ,
I set colmuns to 6 , and I have 7 images total ,
the second row image is bigger then the other 6... and i DON'T understand why!

    const PHOTO_SET = this.props.images.map(img => ({
      src: img.url,
      width: img.width,
      height: img.height,
    }));

**width and height it's the original image size

<Gallery
     columns={columns}
     photos={PHOTO_SET}
    margin={40}   />
    

this is the result , the second row is wrong..
https://i.imgur.com/DipdH2U.png

Can I change anyway the row height of images or it's also automatically , I just want a gallery with thunmbail with normal height and width ?

thanks again

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