miromannino / Justified-Gallery

Javascript library to help creating high quality justified galleries of images. Used by thousands of websites as well as the photography community 500px.
http://miromannino.github.io/Justified-Gallery/
MIT License
1.68k stars 299 forks source link

Last row "center" problem #274

Closed jefftucker1952 closed 4 years ago

jefftucker1952 commented 6 years ago

If the last row is set to "center," but the row is actually the full width of the gallery, it often ends up being one pixel off, usually to the right, but occasionally to the left. If I change to "justify," that last row snaps back to where it belongs. If I change to "nojustify," it also works properly, because it's within the justifyThreshold, so it gets justified anyway.

In short, it appears that if the last row is set to "center," the justifyThreshold isn't used to alter the behavior. It probably should respond the same way that "nojustify" does - if the width is within the justifyThreshold, forget about centering it, and simply justify it. I believe that would cure the problem.

(BTW, the documentation says that the default value for justifyThreshold is 0.75, but in the code it looks like it's 0.90.)

A couple of screenshots:

lastrow01 lastrow02

jefftucker1952 commented 6 years ago

A related thought.... I suspect the centering is off because of a rounding error somewhere - something being rounded that should be truncated or vice-versa, or a float being passed to the browser, leaving it to the browser's tender mercies to figure out how to deal with it. Might be very dicey to fix, since I've observed slightly different behavior in different browsers.

With a typical centered last row, the rounding error is of no consequence, since it's visually not detectable. It's only noticeable when the last row is actually full width. And it becomes even more noticeable if you have a 1px white border on images against a black background - then it's very obvious.

biziclop commented 6 years ago

Hi, could you please make a working example? I can't reproduce this bug.

jefftucker1952 commented 6 years ago

Happy to oblige. You can produce the effect in Chrome or Firefox by narrowing the browser window until you've got 4 rows, fully justified. Then gradually narrow the window - there will come a point at which the last row is shifted 1px to the right. In this example, I've set margins and border to zero, which makes it easier to spot the problem when it occurs.

(demo removed)

I'm not a Javascript guy, and am not adept at tracking down what's happening, but I have a weak theory. I think the last row actually is being justified, but then, because it's set to "center," the script is positioning it by dividing the available width by 2, independently of the other rows, which sometimes triggers the rounding error.

jefftucker1952 commented 6 years ago

Here's another illustration of what I suspect is the related centering problem. In this case, the thumbnails are all a fixed shape - 240x240px:

(demo removed)

Narrow the browser so you have 4 thumbs in the first row and 2 in the second. Try moving the width in small increments - you will eventually land on a spot where the thumbs don't line up. The central "junction" is where the problem is easiest to spot.

jgcenter01

biziclop commented 6 years ago

You're kind of right, at jquery.justifiedGallery.js line 405:

      availableWidth -= Math.round(newImgW);
      $entry.data('jg.jwidth', Math.round(newImgW));
      $entry.data('jg.jheight', Math.ceil(newImgH));

But if I un-round them, they will be aligned vertically, but the gap size will jumpy. It will need some re-thinking of the layout algorithm. :(

jefftucker1952 commented 6 years ago

But remember, it works correctly if you specify lastRow: "nojustify". In that case, you never see the 1px error. That makes me think that the problem is confined to something that's happening only when you specify "center" for that last row. I'm looking at something like lines 443-4 of that file:

if (settings.lastRow === 'center')
        offX += availableWidth / 2;

(BTW, for a Java person, the notion that all numbers in Javascript are floating point is very disorienting!)

Edit: Doing a Math.round() on that does seem to cure the first problem, with the fully-justified last row. Just a quick test, but I'll tinker some more. But it doesn't help the second, more challenging case, with the fixed-shape thumbnails - there, you still get frequent mis-matches where 4 thumbs come together.

jefftucker1952 commented 6 years ago

Just checking in to confirm that changing lines 443-4 seems to cure the original problem that I posted, without producing any unpleasant side-effects, as far as I can tell:

      if (settings.lastRow === 'center')
        offX += Math.round(availableWidth / 2);

Alas, the second problem, regarding alignment of fixed-shape thumbnails, remains. Then again, if you're using fixed-shape thumbnails, you don't really need the JG script at all - regular inline-block layout is robust and infinitely simpler!

jefftucker1952 commented 5 years ago

Sorry to see that this change hasn't made it into the 4.0.0-alpha code. It really is just a one-liner, it really does fix the error described in the first post, and it does not have any undesirable side-effects, AFAIK.

miromannino commented 5 years ago

Thanks @EarlyOut I will definitely include in the code as soon as I fix other things

jefftucker1952 commented 4 years ago

Is Justified-Gallery now abandonware? Asking for a friend.

miromannino commented 4 years ago

Hello @EarlyOut, thanks for coming back here. I wouldn't say abandonware, more about I don't have time for support for people that let's say just have some !important CSS rules that destroy everything (which is 99% reason of any "bug" in JG).

The idea of version 4 was to completely remove jQuery dependency, but it turned out to be more problematic than ever to support various browsers, plus not much time for changes. I would say I will do a 4.0 with your fixes? You need them?

Otherwise JG works as usual, there are just not new features.

jefftucker1952 commented 4 years ago

Thanks, but I've got my manually patched copy of 3.7.0 (this little fix, and https://github.com/miromannino/Justified-Gallery/pull/286), which is working without any issues. I'm just leery of having my own software rely on third-party packages that disappear beneath the waves, having been burned by that in the past. ;)

miromannino commented 4 years ago

Perfect, then. I think at this point I will do as I said, a version 4 with just these fixes soon. Removing jQuery completely doesn't make sense a this point, I would rather write something completely new instead that only works with modern browsers.

Thanks for asking and for your patience!