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

list not displaying content when filtered by custom attribute #60

Closed
Thanood opened this issue Jun 8, 2016 · 18 comments
Closed

list not displaying content when filtered by custom attribute #60

Thanood opened this issue Jun 8, 2016 · 18 comments
Labels

Comments

@Thanood
Copy link

Thanood commented Jun 8, 2016

When filtering a list using value converters (not sure if this is the cause, but anyway) the resulting list is essentially empty. The data is still there (this.items contains the correct data).
In addition to being "empty" the list is displayed in a different order when removing the filter.

It's reproduced here: https://github.com/Thanood/ui-virt-filter-issue

Steps to reproduce:

  • select an item
  • scroll down a few pages (to trigger mutations)
  • select another item
  • enable "show only selected"
  • disable "show only selected"

It's interesting that if you scroll to the top before enabling the filter, everything is okay.

@Thanood Thanood changed the title list "loses" content when filtering list not displaying content when filtered by custom attribute Jun 17, 2016
@Thanood
Copy link
Author

Thanood commented Jun 17, 2016

@vegarringdal mentioned to me that

the reason why that is failing is because nothing is checking if scrolltop is higher then current collection * rowheight

I have not been able to check that, yet.

@Thanood
Copy link
Author

Thanood commented Jun 17, 2016

Please note that the above quote is an assumption, not a fact. 😄

@martingust martingust added the bug label Jun 26, 2016
@AStoker
Copy link
Contributor

AStoker commented Jul 11, 2016

Just to clarify what's going on. You have a list of items, you scroll down a bit so that there are items that are no longer in the DOM "above" where you're looking. You then trigger a filter that filters the items in the list so you have less items than before, but your scroll position stays in the same location, and therefore you don't see any items. But if you scrolled to the top, you'd see everything correctly. Is that correct?

@Thanood
Copy link
Author

Thanood commented Jul 11, 2016

@AStoker I'm not sure if that's correct.. 😄

  • I have a list of 1000 items, let's say 20 per page. Each of them with a chekbox and title. I select two of them.
  • I scroll down to entries 500-520 and select 510 and 511.
  • I click "show only selected" => the list is empty (so less items, yes) apart from a single empty checkbox without any title.

Now with scrolling up before triggering the filter:

  • I select two items from the first 20 entries.
  • I select items 510 and 511.
  • I scroll up.
  • I activate the filter => I get a list with my selected four items (this is the expected behaviour).

@Thanood
Copy link
Author

Thanood commented Aug 10, 2016

Update: in the first case, if I scroll to the bottom (this.listElement.scrollTop = this.listElement.scrollHeight) and then back to the top (this.listElement.scrollTop = 0;) I'm getting the expected result..

Currently failing to do that as a workaround in my app..

@Thanood
Copy link
Author

Thanood commented Aug 29, 2016

Can I do anything to help with this?
I've tried to look at the code myself but I must confess that I don't really get it.

Last time I've tried the following:

this.list.scrollTop = 0;
window.setTimeout(() => {
  this.showOnlySelected = !this.showOnlySelected;
}, 500);

(500ms was really needed in this case)
So I think that it has some relationship to scrolling which I don't understand.

To be more precise:
I mutate the array using a valueConverter. The array is suddenly much smaller and maybe (just a wild guess) the (virtual) scrolling position is "preserved". I guess this is what @AStoker suggested. Now the view shows "page 3" while the "real" list is scrolled to the top (because there is no scrolling).

Result:
image

So the new array is rendered but there are no checkboxes (should be filled) and no names (right to the checkboxes).

@vegarringdal
Copy link

Hi, gave this a try.

If I replace let first = repeat._getIndexOfFirstView();
With this:
let first = Math.floor(repeat.scrollContainer.scrollTop/repeat.itemHeight);

I get @Thanood filtering to work, this change will calculate what the first index should be compared to scrolltop, because it was no correct when you had scrolled when I tried.

I have no idea if I broke something else with this...

BTW, would have been nice when running the sample, if the source maps for ui-virtualization was included when running downloading and running the sample, so much easier to debug, but again think I have to fix this in aurelia-v-grid too:joy:

@Thanood
Copy link
Author

Thanood commented Aug 31, 2016

I can confirm that @vegarringdal's change solves this problem for my use-case. 👍 😄

@Thanood
Copy link
Author

Thanood commented Dec 19, 2016

This is still bugging me. 😃
So, I dug a little deeper and it seems that let first = repeat._getIndexOfFirstView(); is called before the array mutation takes place.
To be more exact, the array is mutated but viewSlots seems to be at its old value.

If you put a log line in there which prints this.viewSlot.children.length you get the following states:

  • initial render, list with 1000 items, 18 visible: 1 child (why 1? initial empty slot?)
  • select 2 items, filter by selected, list with 2 items, 2 visible: 81 children (<-- 81 is the "real" number of viewSlots at initial render)
  • remove the filter, list with 1000 items, 18 visible: 2 children (<-- obviously, this is the "real" amount at the previous step)

If you have a list with 81 viewSlots and have scrolled down, then get the first one you receive the first visible index (say it's 40). That index is inevitably out of bounds for a list (then filtered) with less than 40 items. 😃
If I find some time today, I'll check more.

I'm wondering.. Shouldn't the viewSlot $index be updated after this happens?

    while (viewsLength > itemsLength) {
      viewsLength--;
      repeat.removeView(viewsLength, true);
    }

https://github.com/aurelia/ui-virtualization/blob/master/src/array-virtual-repeat-strategy.js#L34-L37

Sorry, if that's a stupid question, the internals still escape me somehow. 😃
Anyway, this leads to the first index not being at 0 - assuming the filtered list has f.i. two items and thus the list is scrolled up.

@Thanood
Copy link
Author

Thanood commented Dec 19, 2016

It works as expected when you return the actual index of the first view in the viewSlot array as opposed to the index of its overrideContext. So, if you replace this:

  _getIndexOfFirstView(): number {
    return this.view(0) ? this.view(0).overrideContext.$index : -1;
  }

https://github.com/aurelia/ui-virtualization/blob/master/src/virtual-repeat.js#L404-L406

With this:

  _getIndexOfFirstView(): number {
    return this.view(0) ? this.viewSlot.children.indexOf(this.view(0)) : -1;
  }

It works as I would expect it.
Unfortunately, two tests (infinite scroll) are failing with this change. 😕

EDIT: These two tests also fail (in the same way) when I revert the change I did. 😃
Btw. these are the two failing tests:

  • passes the current index and location state
  • passes context information when using call

Thanood added a commit to Thanood/ui-virtualization that referenced this issue Dec 19, 2016
@Thanood
Copy link
Author

Thanood commented Dec 19, 2016

Does that even make sense..? It's always at 0, right? 😂

@vegarringdal
Copy link

@Thanood
What about the workaround/hack I did, did that fail somewhere ?

@Thanood
Copy link
Author

Thanood commented Dec 19, 2016

@vegarringdal I'm not sure about its side effects and there has been no comment about it. (or in other words: not sure if it breaks anything 😏 )
I'll put it to test tomorrow.

@AStoker
Copy link
Contributor

AStoker commented Apr 3, 2017

@Thanood, can you see if #107 fixes this?

@Thanood
Copy link
Author

Thanood commented Apr 3, 2017

I will try it when I get home. 😃

@Thanood
Copy link
Author

Thanood commented Apr 3, 2017

@AStoker I can confirm this issue is fixed using my reproduction repo: http://github.com/Thanood/ui-virt-filter-issue

✨ 👍

@AStoker
Copy link
Contributor

AStoker commented Apr 3, 2017

Thanks! I'll try and get this in shortly

@AStoker
Copy link
Contributor

AStoker commented Apr 5, 2017

Resolved (hopefully) by #107

@AStoker AStoker closed this as completed Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants