Skip to content

fix shelf-first visibleStartingIndex for empty content#32

Open
mukilane wants to merge 1 commit into
stefanpenner:masterfrom
mukilane:master
Open

fix shelf-first visibleStartingIndex for empty content#32
mukilane wants to merge 1 commit into
stefanpenner:masterfrom
mukilane:master

Conversation

@mukilane

@mukilane mukilane commented Mar 4, 2021

Copy link
Copy Markdown

Fixes: #26

@mukilane

mukilane commented Mar 4, 2021

Copy link
Copy Markdown
Author

@stefanpenner Please help resolve the tests.

@mukilane

Copy link
Copy Markdown
Author

@rwjblue / @stefanpenner Could anyone have a look on this ?

Comment thread tests/index.js
});
});

var contentC = [

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This appears to pass even if the fix is removed.
Without a reproduction of the problem, I don't really understand the fix here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@stefanpenner The issue was that at line 228, we divide by index, which is content.length. If suppose the content array is cleared, the subsequent calculations are affected, leading to entry being undefined and causing the #26

This was reproduced in the test.

However, right now, after rebasing, I am getting stuck at an infinite loop at 246.

But the fix is to avoid the NaN.

@stefanpenner stefanpenner Nov 27, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@mukilane in this scenario would it make sense to short-circuit the function, and return the expected visibleStartingIndex?

It doesn't seem like we need to loop anything if the list is empty, or the height is 0.

I haven't tested this yet, but it seems like:

if (height === 0 || length === 0) {
  return 0;
}

WDYT?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@stefanpenner I wonder if a non-empty list would return a height 0.
Given that if length is 0, height would also be 0, so checking length would be fine, unless there is an odd case where entry's height is set to 0. 🤔

However, adding the height condition is fine with me, just in case.

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.

Error when topOffset is not null en the collection is empty

3 participants