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

Sort doesn't work well with detail row #127

Closed
ronnetzer opened this issue Oct 29, 2020 · 5 comments
Closed

Sort doesn't work well with detail row #127

ronnetzer opened this issue Oct 29, 2020 · 5 comments
Assignees
Labels
comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) has PR type: feature type: refactor
Milestone

Comments

@ronnetzer
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Owner

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

ezgif com-gif-maker

@ronnetzer
Copy link
Contributor Author

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
Copy link
Owner

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 shlomiassaf added comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) type: feature type: refactor and removed waiting for input labels Nov 2, 2020
shlomiassaf added a commit that referenced this issue Nov 3, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
@shlomiassaf
Copy link
Owner

@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.

shlomiassaf added a commit that referenced this issue Nov 10, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
shlomiassaf added a commit that referenced this issue Nov 10, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
@shlomiassaf shlomiassaf added this to the 3.0.0 milestone Nov 10, 2020
shlomiassaf added a commit that referenced this issue Nov 11, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
shlomiassaf added a commit that referenced this issue Nov 15, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
shlomiassaf added a commit that referenced this issue Nov 15, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
shlomiassaf added a commit that referenced this issue Nov 16, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
shlomiassaf added a commit that referenced this issue Dec 3, 2020
feat(ngrid/detail-row): integrate detail row state with context (#10)
fix(ngrid/detail-row): sort doesn't work well with detail row (#127)

Breaking Change
Default `whenContextChange` is now `context`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) has PR type: feature type: refactor
Projects
None yet
Development

No branches or pull requests

2 participants