Starcounter / Starcounter.Uniform

Helper library for server-side part of uniform components.
MIT License
0 stars 0 forks source link

Expand the API of uni-data-table helper to allow external operations on rows #7

Open PatrykSzwer opened 6 years ago

PatrykSzwer commented 6 years ago

In the current state of our API, when user defines his row view-model and implements some methods to manipulate with it, e.g delete it, uni-data-table won't reflect nor react on those changes. e.g, users row view-model: .json

{
  "FirstName": "",
  "LastName": "",
  "Email$": "",
  "DeleteTrigger$": 0
}

c# {

    partial class DataTableRow : Json, IBound<TableRow>
    {
        public void Handle(Input.DeleteTrigger action)
        {
            this.Data.Delete(); // This will not be visible on data table and we don't have any method to invoke removing row from it.
        }
    }

}

warpech commented 6 years ago

My comment https://github.com/Starcounter/UniformDocs/pull/338#discussion_r203504578 contains a solution that works when inserted in the trigger function.

One might wonder what is actually the desired solution?

I think that it should be:

  1. Remove the deleted row from its page.
  2. Add more rows to the page so that the page has the desired number of rows again.

My solution linked above only implements point 1.

PatrykSzwer commented 6 years ago

Yes, this will be a solution but not really elegant one. Our initial plan was to expand API to allow user do such additional actions easily.

But I guess for the current solution in UniformDocs it will be enough.

warpech commented 6 years ago

I think there is also a bug in Starcounter.Uniform helper. Please read my steps to reproduce in https://github.com/Starcounter/uniform.css/issues/158#issue-342467389.

As you can read in that issue, the patch that comes from the server is:

[{"op":"replace","path":"/_ver#s","value":1},
{"op":"test","path":"/_ver#c$","value":1},
{"op":"remove","path":"/UniformDocs_0/CurrentPage/UniformDocs_0/DataTable/Pages/0/Rows/2"}]

What is missing here from the server is one more operation - to replace the number of TotalRows. I would expect this to be the response from the server:

[{"op":"replace","path":"/_ver#s","value":1},
{"op":"test","path":"/_ver#c$","value":1},
{"op":"remove","path":"/UniformDocs_0/CurrentPage/UniformDocs_0/DataTable/Pages/0/Rows/2"},,
{"op":"replace","path":"/UniformDocs_0/CurrentPage/UniformDocs_0/DataTable/TotalRows","value": <new number of rows>}]
PatrykSzwer commented 5 years ago

I was working on that quite a bit (along with @alshakero, thanks for help with the frontend part) and we have some conclusions.

The idea behind it was to not force the user to use loadRows and his own implementation for deleting rows. I tried to provide such behavior, by adding method that is deleting a row and removing it visually from the table also. But this has some flows:

Basically what I think would be the best, is to put more effort into a UX part instead of performance. I would rather implement delete row method, which would simply delete row data from the table and reload it, instead of messing around with deleting only a single row and not doing anything else. By that we will have nice UX where when you delete a row, everything will be moved up and new row from the next page will be presented. @warpech let me know if you agree with this approach, me and @alshakero are strongly advising it.

warpech commented 5 years ago

I would rather implement delete row method, which would simply delete row data from the table and reload it, instead of messing around with deleting only a single row and not doing anything else. By that we will have nice UX where when you delete a row, everything will be moved up and new row from the next page will be presented.

We discussed this and I agree with it, with the following remarks: