Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

[NG] Ability to set the state of a datagrid with a single input #2094

Closed
youdz opened this issue Mar 20, 2018 · 20 comments
Closed

[NG] Ability to set the state of a datagrid with a single input #2094

youdz opened this issue Mar 20, 2018 · 20 comments

Comments

@youdz
Copy link
Contributor

youdz commented Mar 20, 2018

Select one ... (check one with "x")

[ ] bug
[x] feature request
[ ] enhancement

Expected behavior

We need to offer a [(clrDgState)] two-way binding on the Datagrid component itself, to allow people saving a datagrid's state to the URL or local storage to easily reinitialize a new one in the same state.
This feature would be an excellent initial compromise for #377.

Actual behavior

Currently, we offer a (clrDgRefresh) output that emits the entire state, but the only way to use that state to initialize the datagrid is to individually assign the filters, sort, current page, ...

@youdz
Copy link
Contributor Author

youdz commented Mar 20, 2018

I'm going to open this to external contributions because it's a fairly straightforward feature to add:

  • The StateProvider for Datagrid has a getter for the state, all that's needed here is to add a setter with the same name, that sets the proper values in the various Page/Sort/Filter providers. The code is almost the same as for the getter, iterating over the same objects.
  • ClrDatagrid component will receive a new @Input("clrDgState") that uses the setter from the StateProvider, and a @Output("clrDgStateChange") that is an exact copy of clrDgRefresh (which we will keep for backward compatibility).

@gssjr
Copy link

gssjr commented Mar 23, 2018

One thing I noticed is that when sorting by a column that uses a custom comparator, the grid state will store a reference to the comparator instead of the column reference. Similar with filters. Does the state need to know that there is a custom comparator on that column? Or just that it is sorted by that column?

@youdz
Copy link
Contributor Author

youdz commented Mar 30, 2018

The main case for emitting the state is Datagrids using *ngFor, where we don't handle the sorting/filtering ourselves, but let the consumer or server do it.
Because of this, we do have to emit what the custom sort comparator is, otherwise you'd have to look it up base on the column every time, which isn't as straightforward as using the comparator we just gave you.

@juanmendes
Copy link

juanmendes commented Apr 6, 2018

When this is implemented, would it be possible to provide a way for it not to send notifications a la FormControl#setValue. We currently have to hack it really hard to be able to ignore events that are caused by setting page/filters.

We set page/filters programatically because we have a card view and a grid view of items and we have to synchronize the pages/filters (all custom filters) between both views.

audax added a commit to Qabel/clarity that referenced this issue Nov 22, 2018
Add an input and output for the state of the datagrid. The output is the same
as existing clrDgRefresh.

Closes vmware-archive#2094

Signed-off-by: Jens Kadenbach <[email protected]>
@audax
Copy link

audax commented Nov 22, 2018

I am currently working on this issue, but I have hit a road block. I can't find an easy way to set the filter state or at least set a filter as inactive in a generic way.

The solution I currently implemented is to unregister all filters that are not === to the filters in the state (with the special case of the builtin string filter), but I fear that it may have side effects, especially because we are dealing with mutable objects, which are not a great fit for setting state.

My use case only involves server side filtering, so I may just end up not using the Clarity filter components at all, but apart from that problem, my code is working fine.

https://github.com/Qabel/clarity/tree/datagrid-state-input

audax added a commit to Qabel/clarity that referenced this issue Nov 22, 2018
Add an input and output for the state of the datagrid. The output is the same
as existing clrDgRefresh.

Closes vmware-archive#2094

Signed-off-by: Jens Kadenbach <[email protected]>
audax added a commit to Qabel/clarity that referenced this issue Nov 22, 2018
Add an input and output for the state of the datagrid. The output is the same
as existing clrDgRefresh.

Closes vmware-archive#2094

Signed-off-by: Jens Kadenbach <[email protected]>
audax added a commit to Qabel/clarity that referenced this issue Nov 22, 2018
Add an input and output for the state of the datagrid. The output is the same
as existing clrDgRefresh.

Closes vmware-archive#2094

Signed-off-by: Jens Kadenbach <[email protected]>
@audax
Copy link

audax commented Nov 26, 2018

Small update and bad news: Configuring sorting and filters is a pain, my tests in the branch are not complete. When I set the sorting for example, I need to find the comparator object and toggle it, but since I don't have a reference to it, this task becomes very tedious.

My code therefore only really works for pagination and is therefore not really useful.

@speti43
Copy link

speti43 commented Nov 28, 2018

Hi @audax, @youdz ,

We also need this feature and we would like to contribute, but I'm a little bit worried seeing the discussion above :) Is it still possible to implement the 2-way binding, in the current code structure, or a major refactor is needed to implement it? Any suggestion what needs to be done to make it work?

Thanks,
Peter

@youdz
Copy link
Contributor Author

youdz commented Nov 28, 2018

The reason it's bit stting there for so long is indeed because it requires a refactor. Off the top of my head, I'd expect to need a new equals() method on both the filter and the comparator interface. Otherwise we would only be able to use filters and comparators received as part of the input through reference equality, which never works for stored state. This means updating the code in all places that use these filters and comparators.
It's not that bad, but because this is the datagrid which accumulated a lot of legacy of the years, even a small refactor can take much longer than anticipated.

One tricky piece of code is also going to be the swap logic in the filters provider to swap an old filter for a new one, as opposed to just adding it. The important part will be to make sure there is no memory leak due to unused subscriptions.

Finally, we have slightly anticipated on this by creating a getter for the state in the StateProvider, so the input logic will simply go in the corresponding setter. At least that's something that won't need a rewrite.

@audax
Copy link

audax commented Nov 29, 2018

@speti43, my way of syncing the state is now to only sync the sorting and pagination, which I do by directly binding to the inputs of those elements and the state output of the datagrid. Since I only use server side filtering, I don't even bother with the datagrid filters.
To implement the state input, you would need to implement an equals method, preferably a "state" input/output and and an equals method for comparators and filters, as well as methods to retrieve all registered comparators and filters of the datagrid. Swapping filters and comparators would also mean adding those to the UI, so I would restrict the state input to only activate/deactivate and configure already registered filters and comparators.

It would very much be a breaking change and since clarity already is close to 1.0, it's probably to late for that, but it would have been a good time to do it.

@speti43
Copy link

speti43 commented Nov 29, 2018

@audax, sounds good. I've just forked the repo with your changes. I don't think it will go into 1.0.0 :) But this don't necessarily have to be a breaking change, or should we deprecate the refresh emitter?

@speti43
Copy link

speti43 commented Nov 29, 2018

@audax, @youdz, I created a pull request, with some comments, it's not ready to merge, just I wanted to ask for code review. Please read the PR comments.
Qabel#1

@audax
Copy link

audax commented Nov 29, 2018

As I posted into the PR on my fork: I think a neat solution would be to have 2 extra filter and comparator interfaces with the required methods and ignore all other filters and comparators that do not implement equals() and other required methods or maybe just throw an exception in that case.

This of course increases the complexity of the feature by quite a bit.

@amellnik
Copy link
Contributor

@youdz Can we have a new feature branch for this on top of #2846? It looks like it should allow the current draft PR to be simplified.

@youdz
Copy link
Contributor Author

youdz commented Feb 26, 2019

I missed this comment earlier, sorry. Topic branch created: topic/datagrid-state-input

@Kangcor
Copy link

Kangcor commented Apr 30, 2019

Is this feature request under external development? Seems like topic branch is really behind...

We would love this feature to simplify the navigation back to a datagrid with filters and pagination applied in our project.

@speti43
Copy link

speti43 commented Apr 30, 2019

Me and my colleague have stopped the contribution, our project which used clarity had ended. I don't know if someone has continued the development.

@AliWieckowicz
Copy link

Is there any work-around for presetting state properties for the data grid?

@amellnik
Copy link
Contributor

I was hoping to work on this after #2996 is merged -- I think that plus #2846 (already merged) significantly reduces the scope of this.

@gnomeontherun
Copy link
Contributor

This is a long standing request that we understand would make things easier in some cases, but also is a major overhaul and potential breaking change to implement. As we build out Clarity Core version of the Datagrid, we expect that an application could easily wrap it and make this feature for their own use case and not be blocked by us in any way. Closing as something we aren't planning to tackle for Clarity Angular.

@github-actions
Copy link

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants