Open OrkhanAlikhanov opened 3 years ago
@OrkhanAlikhanov Greets and congrats, you are the first issuer of the vscroll repo! Your request seems quite important, but I have one doubt, the point is that the scroller engine has its own asynchronicity related to updating the DOM:
This is the Render process, the heart of the VScroll Workflow. I would agree, it looks not perfect, but it should be enough to catch 1 tick. I wonder if you need more time... We can think about how to control this asynchronicity, something like
new Workflow({
element: ...,
datasource: ...,
run: ...,
onRender: done => this.$nextTick(() => done()) // default: done => setTimeout(() => done())
})
and then replace setTimeout
in the Render process:
onRender(() => {
if (Render.doRender(scroller)) { ...
...
})
Also, there should be a way to cancel render, something like onRender: (done, off) => ...
. This is interesting story. Do you want to try this with me? I'd gladly participate in Vue integration (never dealt with Vue), and it would be nice if we could schedule a 2-hours meeting. What do you think? Send me your time (when available) if you like the idea: dhilt.public@gmail.com
Hey! Thank you for quick response! That approach seems more appropriate way of solving this. Having said that, I patched the library locally and rendering works, but now I'm running into another issue where reload
is inconsistent. Calling reload(n)
does result in updating dom with specific nodes but it keeps display: none
on some, if not all, item elements on the screen. It turned out to be an issue with Buffer.reset
, specifically this.item = []
line. That assignment results in calling run
which does not immediately update DOM. I made it await run
method to complete rendering and seems to work now. What do you think?
Thank you for the meeting offer 🙂, I'd like to dive into this a bit more and get it working with Vue so that I can become more helpful in the Vue integration process
I couldn't reproduce late rendering issue except locally, but was able to reproduce reload issue. Would be great if you could take a look at it, maybe my setup is broken. Let me know if you have any questions. https://stackblitz.com/edit/vue-vscroll
@OrkhanAlikhanov Well, vscroll v1.0.0 have been just released, a new chapter of this story begins, and I'd love to have onRender
updates in 1.1.0. Do you have your efforts published somewhere?
You are right with Adapter.reload -> Buffer.reset -> Workflow.run call stack.
Line 45 is responsible for display-none, line 47 forces Workflow.run to be invoked, and as it follows from the doc (run-callback requirements section):
old items that are not in the list should be removed from DOM
Explicit hiding occurs internally, it is synchronous and allows to avoid any blinking. But physical removal is a consumer responsibility, it means when you get [] in Workflow.run, you have to remove all items from DOM by yourself, using items[].element
references.
PS Feel free to ping me by email, I'm pretty interested in Vue, though I still know nothing about it 😁
@OrkhanAlikhanov I looked at your stackblitz. The main problem with reload
as far as I see is how Vue detects changes. The following is my assumption, not knowledge. The line :key="item.nodeId"
provides vue-template identity, and when you do reload staying at 0 position, the first rows after reload and before reload will be the same from the Vue's point of view, because theirs "key" props will not change. To prove it, you may scroll down before reload (for the first row key to be greater than 0), and then reload will work properly.
The suggestion is quite simple: add reload counter to :key="item.nodeId"
value. Something like :key="prefix + item.nodeId"
, where
prefix = workflow.scroller.adapter.id + '.' + workflow.scroller.adapter.reloadCounter + '.';
or (equivalent)
prefix = workflow.scroller.adapter.reloadId + '.';
This will guarantee key uniqueness across reload/reset.
In addition I would state a few basic points that you should implement in your project:
this.datasource
), not in the Scroll one; instantiate Datasource in the App module and pass it to the Scroll module as a param; it is important due to separation of concerns, and this will let you to extract vue-vscroll module into a separate generic solution (a consumer in terms of vscroll);this.datasource.adapter.reload(this.reloadIndex)
Thank you for taking a look at it. That prefix
will definitely solve it 💪🏽
I think I know why it happens. The way vue renders is through diffing 2 virtual DOMs and patching the differences. Before reload, DOM has everything in place from item 0 to 10 let's say. When we hit reload, this.items.forEach(item => item.hide());
updates each element
with display: none
and eventually calls run
method where we set new the given items as the new data and that triggers rendering. Vue generates new virtual dom, and compares it with the new one. It sees no difference and therefore does not update anything and that display: none
sticks around.
Giving :key="prefix + item.nodeId"
, will solve the problem because when vue diffes the virtual doms, it compares the key
value in the first place. Different keys in the virtual doms will force vue recreate the element.
What do you think about vscroll
adding display: none
but not removing it?
Thank you for your suggestions about only exposing datasource
. I have some questions if you don't mind.
adapter.replace
method, but how can I update maxIndex
and startIndex
?reload
method? Currently what I do is call reload
and await isLoading$
to be false, then update window.scrollTop
with correct offset. The reason I need of set is because I have a sticky header.I'm just dumping my experience here to give you some ideas based on my usage. Not trying to force anything into the library 🙂 Thank you for the awesome work!
@OrkhanAlikhanov 👍 That's it. On hiding/removal please refer to this comment. In two words: a consumer must clean up the DOM, but the Scroller is not sure if it will happen immediately and synchronously hides the elements to be removed. Questions:
1) This depends on the data flow you suppose to have in the end App
2) Adapter.reload just reloads the viewport. The following I would use
async doReload(index: number, scrollPosition?: number) {
await this.datasource.adapter.reload(index);
if (scrollPosition !== void 0) {
this.datasource.adapter.fix({ scrollPosition });
}
}
Thank you for your help! You did help me a lot. Here is the state so far https://stackblitz.com/edit/vue-vscroll
reload
does not return a promise, why did you await it above?
When the given vscroll data set changes, UI should be updated accordingly. There can be 2 types of change:
[{id: 1, checked: false}, {id: 2, checked: true}]
becomes [{id: 1, checked: true}, {id: 2, checked: false}]
[{id: 1}, {id: 2}]
becomes [{id: 2}, {id: 1}]
or [{id: 3}, {id: 4}]
For the second case, vscroll has to reload.
For the first case, there is no need to remove and re-add dom elements which causes flashing, we can just let vue update those items on the screen. I tried adapter.update
but it caused flashing issue (because it was adding display: none
), so I resorted to setting this.dataItems[].data
to new data which updates dom. The way I detect the first case is by generating a string based on uniqueKey
value. identity([{id: 1}, {id: 2}])
gives 1-2
:
watch: {
items(newItems, oldItems) {
const identity = (items: any[]) => items.map(x => x[this.uniqueKey]).join('-')
if (identity(newItems) === identity(oldItems)) {
/// just update displayed data
this.dataItems.forEach((item) => {
item.data = newItems[item.$index]
})
} else {
/// else reload completely
this.datasource = this.generateDataSource()
this.workflow.scroller.adapter.reset(this.datasource)
}
},
},
I understand this is not efficient, there are things that can be optimized further like identity
function. Even dataItems
should not be marked reactive as it has performance cost. But it works nicely for my case.
How does ngx-ui-scroll
handle such dataset changes?
@OrkhanAlikhanov I made some updates in your demo: https://stackblitz.com/edit/vscroll-vue-integration. The main point is that the App data is separated from the Vue Scroller component. I had to expose the Datasource class as a component property, hope this doesn't violate standard practices. Please check if it looks in agreement with Vue way, I'd like to have it as a shareable consumer proto-sample....
Adapter.reload does return promise, it resolves when the Scroller stops after the reload process is started and completed. This vscroll functionality has spec in ngx-ui-scroll repo.
Speaking of in-place updates which should not force the rows to fully re-render, I'd suggest to look at Adapter.fix({ updater }) API. It provides access to buffered items and allows modifications without updating the Buffer items array reference (and without Workflow.run call). But as I can see, this also works when we make a simple assignment, like in the demo:
this.items[index].checked = !this.items[index].checked // just don't do `this.items = ...`, the reference must persist
I also removed watch
logic (after I failed to figure out where we are getting richer), for me the result works as expected. In ngx-ui-scroll I totally rely on framework's internal mechanisms with only 1 important thing: I use OnPush change detection strategy rather than default (link, info), that's why I have this line in the Workflow.run implementation.
Hi, I'm running into an issue where Vue updates dom a bit late than the flow expects. Basically the updates happen after
run
callback returns. It would be great if we hadrun
's signature as something like(items: Item[], done: () => void) => {}
where we calldone()
after dom update.What do you think?