shlomiassaf / ngrid

A angular grid for the enterprise
https://shlomiassaf.github.io/ngrid
MIT License
241 stars 41 forks source link

Sort doesn't work well with detail row #127

Closed ronnetzer closed 4 years ago

ronnetzer commented 4 years ago

What is the expected behavior?

When having a detail row and invoking sort of some column, I expect that the detail row will 'move' along with its parent row.

What is the current behavior?

the detail row remain open while only the content changes. If a row without detail row takes the position of a row with an opened detail row, the detail row will not show (which is good, I think). Also tried to override this by using whenContextChanging="ignore" with (toggledRowContextChange) but I guess sorting doesn't change the context as it doesn't emit.

What are the steps to reproduce?

Attaching a gif that demonstrates the issue: (didn't have the time to create a stackblitz, if it won't be clear I'll add one later) Screen Recording 2020-10-29 at 10 18 51

Which versions of Angular, CDK, Material, NGrid, OS, TypeScript, browsers are affected?

latest

Is there anything else we should know?

Also (a bit unrelated so let me know if you would like me to open it as a new issue), it will be nice if creating a custom expand handler will be more like creating custom expand row (PblNgridDetailRowComponent) which exposes exportAs and an expanded getter, it will make it possible to create a reused custom expand handler template.

shlomiassaf commented 4 years ago

There is an old open issue regarding state and detail row however I think it's no longer valid and anyway this has nothing to do with context because once you click sort/filter etc... you invalidate the context and it will get cleared out cause the datasource is changing...

So for server side datasource I think this should not happen.

I assume now that you're talking about client side datasource, if this is the case perhaps there's an issue here. Is this the case?

About your suggestion, this should be easier to do now, after the refactor done for infinite rows, we should be able to provide custom row with own handlers same as you can provide custom invisible row... it's the same mechanism. It should be even simpler in V3. Anyway, open a new issue for that, let's not mix things but also not forget them :)

shlomiassaf commented 4 years ago

I tried on the demo site, looks like it's working. Can you setup a demo online?

ezgif com-gif-maker

ronnetzer commented 4 years ago

you invalidate the context and it will get cleared out cause the datasource is changing

Just to be clear, it's relevant only for server side sort, right? And in such case 'whenContextChange' will be used?

I was referring to client side sort. The gif you've attached shows the problem, instead of opening the row that was opened before the sort, the context of the detail row is changing according to the new 'parent' after the sort.

The issue you linked might be relevant, if each row holds its own state it will be easy to implement what I'm asking

shlomiassaf commented 4 years ago

I understand.

The problem, as I see it, is indeed in the context.

For example, if you "edit" a field and show an input text and scroll down (using virtual scroll) when you go back you want to see it still open in edit mode. This is supported because the state of"edit" is saved on the context, each row has a context.

Detail row does not have this support because it is a plugin, i.e. it does not have a special property set on the context for it to save data.

What is required here is to add a feature for adding custom data that can "live" on the context so all plugins can read/update the context, but in isolation, i.e. they get their space on the context. Once this is done the detail row will be able to use the context. It shouldn't be big, i'll see when I can do it.

Once done, moving a page in my example will close the detail row, going back will open it. Same for sort.

The other issue here is that client side does not support context when you sort/paginate cause in general this action causes context clearance. This is not justified, only server side operations should do that, so i'll have to fix it to.

Hope it's clear.

shlomiassaf commented 4 years ago

@ronnetzer this is the V3 branch, but I think it should fix the issue for you.

https://shlomiassaf.github.io/ngrid/v3/features/built-in-plugins/detail-row#row-updates

Available once V3 is out.