Properly fix wrong offset calculation #382
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 thebuildingRow(while accounting for the aspect ratio a second time) was below therowHeightto flush the row, thereby treatingrowHeightlike a maximum height.It seems the intention prior to #286 was to tentatively add the next entry's aspect ratio, without buffering it in
buildingRowyet, to determine if that'd push us below therowHeight, and if so, flush the row before that next entry, thereby treating therowHeightlike 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 therowHeight.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.