Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Properly fix wrong offset calculation #382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgunther
Copy link

PR #286 attempted to fix a doubled margin between rows, however in my use case, while it solved that, it increased the images per row, thereby reducing the height of each image. Especially with a number of very wide, but short, images, this led each row to be significantly shorter then before #286.

I believe the source of that problem is that after #286, we'd buffer the next entry, add it's aspect ratio in the buildingRow, then determine if the buildingRow (while accounting for the aspect ratio a second time) was below the rowHeight to flush the row, thereby treating rowHeight like a maximum height.

It seems the intention prior to #286 was to tentatively add the next entry's aspect ratio, without buffering it in buildingRow yet, to determine if that'd push us below the rowHeight, and if so, flush the row before that next entry, thereby treating the rowHeight like a minimum height.

In other words, before #286, we'd flush the row BEFORE adding the entry that would push us below the configured rowHeight, but after #286, we'd flush the row AFTER adding the entry that pushed us below the rowHeight.

The root source of the doubled margin was flushing a row with no buffered entries (hence increasing the offset without actually rendering a row). Given an empty buffered entries (start of a new row), if the entry being analyzed had an aspect ratio that'd make it's height less than the configured rowHeight, we'd flush the row BEFORE buffering the entry, thereby flushing an empty row.

Now, we only attempt to flush the row if we have at least one buffered entry.

This should still fix #223 and #275 without introducing the side-effects described above.

PR miromannino#286 attempted to fix a doubled margin between rows, however in my
use case, while it solved that, it increased the images per row, thereby
reducing the height of each image. Especially with a number of very
wide, but short, images, this led each row to be significantly shorter
then before miromannino#286.

I believe the source of that problem is that after miromannino#286, we'd buffer the
next entry, add it's aspect ratio in the `buildingRow`, then determine
if the `buildingRow` (while accounting for the aspect ratio a second
time) was below the `rowHeight` to flush the row, thereby treating
`rowHeight` like a maximum height.

It seems the intention prior to miromannino#286 was to tentatively add the next
entry's aspect ratio, without buffering it in `buildingRow` yet, to
determine if that'd push us below the `rowHeight`, and if so, flush the
row before that next entry, thereby treating the `rowHeight` like a
minimum height.

In other words, before miromannino#286, we'd flush the row BEFORE adding the entry
that would push us below the configured `rowHeight`, but after miromannino#286,
we'd flush the row AFTER adding the entry that pushed us below the
`rowHeight`.

The root source of the doubled margin was flushing a row with no
buffered entries (hence increasing the offset without actually rendering
a row). Given an empty buffered entries (start of a new row), if the
entry being analyzed had an aspect ratio that'd make it's height less
than the configured `rowHeight`, we'd flush the row BEFORE buffering the
entry, thereby flushing an empty row.

Now, we only attempt to flush the row if we have at least one buffered
entry.

This should still fix miromannino#223 and miromannino#275 without introducing the side-effects
described above.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong margin calculation in smallest breakpoint, larger than intended gap
1 participant