vincjo / datatables

A toolkit for creating datatable components with Svelte
https://vincjo.fr/datatables
MIT License
426 stars 14 forks source link

[Suggestion] Add internal variables such as selected & rowId #21

Closed piperun closed 1 year ago

piperun commented 1 year ago

I'm wondering if it's possible to have datatables have rowId & selected as internal variables for each row. It would certainly help with handling dynamic data that may change it's original id and also ease the burden of stapling together selection functionality.

I've poked around the source code and if I understand it correctly, it should be possible but I might be misunderstanding how it works: The only issue I saw is how you implement rowId given the Row class is supposed to hold internal state/variables.

vincjo commented 1 year ago

Seems to me the data should be reprocessed by adding a unique id property before injecting into the "rawRows" store

import { type Writable, writable } from 'svelte/store'

export default class Context
{
    public  rawRows : Writable<any[]>

    constructor(data: any[], params: Params)
    {
        this.rawRows = this.setRawRows(data)
    }

    private setRawRows(data: any[])
    {
        const rows = data.map(row => {
            return {
                dt_uid: Math.random().toString(16).slice(2),
                ...row
            }
        })
        return writable(rows)
    }
}

This risks making the instantiation heavier than it should :thinking: and we'd lose the state after refetching / invalidation

Most of the data includes a primary key and I wonder if we could offer an optional "pk" parameter to manage this

{ rowsPerPage: 50, pk: 'id' }
piperun commented 1 year ago

Yeah the way I've experimented with rowId is by inserting it while processing the rows:

    public applyRowId(): void
    {
        this.rawRows.update((rows) => {
            return rows.map((rowData, i) => ({ rowId: i, ...rowData }))
        })
    }

/*my poor attempt at inserting selected, it "works" but since all rows are ultimately represented
 as a Readable it gets awkward to set selected*/
    public applyInternalVars(): void
    {
        this.rawRows.update((rows) => {
            return rows.map((rowData, i) => ({ rowId: i, selected: false, ...rowData }))
        })
    }

Issue is, since the exposed row is a Readable, it makes impossible to change (maybe you can do some mojo to make it work) their values. What I was thinking... if I understand your code correctly is, perhaps having each row had said id & selected state:

export default class Rows
{
    private rawRows: Writable<any[]>
    private triggerChange: Writable<number>
    private sorted: Writable<Sorted>
    private selected: Writable<boolean>
    private rowId: Readable<number> // or Writable<number

   // rest of Rows class
}

Where you can then simply change selected state via setSelected() and get all selected states getAllSelected()

vincjo commented 1 year ago

Yep, you're all right about the code.

On reflection I don't think internal properties are the best option. I hadn't thought of it but it could cause CRUD problems with certain APIs, if $row object is sent with "rowId" and "selected" props to the backend models.

What about a separate object of type array which store ids for each selected rows? Using "includes" to check for equality instead of a per-line state

I wrote a small off-lib example to illustrate: https://vincjo.fr/datatables/row-selection

There is also the issue of whether selectAll() is for ALL rows or only rows displayed in the current page. I guess it should select every rows

piperun commented 1 year ago

That's a fair point, this is just a big maybe, but I think maybe it could be a good idea to implement CRUD in datatables so that:

  1. You got a standardized way for 3rd-party APIs to CRUD
  2. You get better means to tests if a new feature might impact CRUD

100% understand if it's not part of the goal/philosophy with the project or is going to take up more development time than it's worth (I could try and help out but can't promise I can deliver).

I think the example looks pretty darn good, I think if it could be bundled within DataHandler that would make it a lot more seamless (instead of having a separate library). The only question is then rowId, since rowId is to keep track of the "original position" of data, unless I misread your code, I assume id is still intended to represent the loaded tableData and not the internal id?

What I've seen selectAll() from the different datatables I've used it's usually been visible (filtered via search/filter) only that gets selected (what your example is implemented as).

vincjo commented 1 year ago

Thx for your suggestions

About row selection I took a slightly different path without resorting to internal vars. However it'll be necessary to specify a unique key and its accessor according to the data structure.

The selectAll() scope is configurable: current page or full dataset.

handler.selectAll(accessor: string, scope: 'page' | 'all' = 'page')

The example below tests selection on the whole dataset https://vincjo.fr/datatables/row-selection

piperun commented 1 year ago

The implementation looks good, if I understand the code correctly the only concern is that what if {id: <val>} or whichever unique key that was added/came with the data gets changed?

Also about CRUD, I think tabulator has the most seamless experience with CRUD methods. https://tabulator.info/docs/5.4/update#alter-add

vincjo commented 1 year ago

Sorry I don't quite understand the use case where the id might have changed.

Had a look at the tabulator CRUD methods, thx for sharing. It seems relevant to add some of these methods, for example the partial update would be great instead of reloading the full dataset each time

piperun commented 1 year ago

My bad I was not very clear.😅 My concern is the opposite . In that the row ids aren't created based on the table but from the user it might cause said ids to not be synced up or change during the table's lifetime. Since the select example shows data: {"id":1,"first_name":"Tobie","last_name":"Vint","email":"tvint0@fotki.com"} but that id is generated by the user/database/another source and can change and thus any selection on row id 1 will instead select something entirely different:

{"id":1,"first_name":"Tobie","last_name":"Vint","email":"tvint0@fotki.com"}
{"id":3,"first_name":"Gérianna","last_name":"Bunn","email":"gbunn2@foxnews.com"},

row_fetch_id_1 -> first_name: Tobie database changes id:

{"id":1,"first_name":"Gérianna","last_name":"Bunn","email":"gbunn2@foxnews.com"},
{"id":3,"first_name":"Tobie","last_name":"Vint","email":"tvint0@fotki.com"}

row_fetch_id_1 -> first_name: Gérianna Again I could misinterpret the code, if so I do apologize.

Tabulator is a really amazing datatables library, sadly it's not very easy to work with on svelte (written for vdom in mind), so the more feature you can implement from there the better!

vincjo commented 1 year ago

sorry for the late response

Very clear explanation.

I have to determine what this implies in terms of data life cycle, code maintainability, Run some tests etc.

For the moment I had no difficulties in doing CRUD but I keep the partial update in mind as a relevant devlopment for the future

piperun commented 1 year ago

Apologize for the late response! I see that row selection now is implemented and I have to say a big thank you for implementing it!

vincjo commented 1 year ago

Thank you for the discussion