swimlane / ngx-datatable

✨ A feature-rich yet lightweight data-table crafted for Angular
http://swimlane.github.io/ngx-datatable/
MIT License
4.63k stars 1.68k forks source link

Table (view) is not update when the rows are updated #255

Closed Caballerog closed 7 years ago

Caballerog commented 7 years ago

I'm submitting a ... (check one with "x")

[ X] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here

Current behavior

I've a table which I update using a method like newRow

newRow(){
  this.rows.unshift({
      price: 100,
      duration: 60,
      state: 1,
     });
}

where

<datatable #datatableTeaching class="material" 
            [rows]="rows" 
            [columnMode]="'force'"
            [headerHeight]="auto"
            [rowHeight]="'auto'"
            [footerHeight]="'auto'"
            >
....

But the view (table) is not update since that I resize the window. Although the element is pushing in the array.

The problem is the same when I remove the element from the row.

Expected behavior

The view (table) and the array is updated with a new element.

Reproduction of the problem

What is the motivation / use case for changing the behavior?

I need update a table in real time for user interaction, which works in the old version 0.6.1.

shattar commented 7 years ago

I believe this is how the OnPush change detection strategy works. You may need to markForCheck. See http://blog.thoughtram.io/angular/2016/02/22/angular-2-change-detection-explained.html

Caballerog commented 7 years ago

Hello @shattar,

I thought that the problem could be the OnPush change detection strategy too. So, I was search the way to invoke manually to refresh the DOM, but I didn't found it.

I've tried this code (in my component which use angular2-data-table) but this doesn't work:


import {  ChangeDetectorRef } from '@angular/core';

constructor(
     private cd: ChangeDetectorRef) {  }

newRow(){
  this.rows.unshift({
      price: 100,
      duration: 60,
      state: 1,
     });
     this.cd.markForCheck(); // marks path
}

Should I change my component or the angular2-data-table code? (I guess angular2-data-table module).

Thanks!

shattar commented 7 years ago

I guess markForCheck doesn't cascade down to the child components. I recreated the issue on plnkr (without even using <datatable>).

https://plnkr.co/edit/rAWUWYXZ5bX14WQ0pEpb?p=preview

I'm not aware of how to trigger a child component to check for change without using an immutable input (copy the array to a new one and change the reference), or tickling it with a purpose built update method/event that a parent component can call/emit. Both seem ugly. I would love to know an elegant solution to this!

amcdnl commented 7 years ago

The change detection strategy is onOnPush, u need to push a whole new rows collection. Modifying the existing collection does not trigger updates.

Like this: https://github.com/swimlane/angular2-data-table/blob/master/demo/basic/filter.ts#L69

shattar commented 7 years ago

This is definitely the way ng2 is intended to work, however, it feels a bit awkward for this particular component... seeing as how the main function is to render an array to the screen.

See the comments on issue #32. The filter demo method has some other undesirable side effects, I mean... it filters properly, but... sort the list, then type in a filter, sorting breaks.

Kind of caught between a rock and a hard place. We want OnPush for efficient change detection, but that will cause us to create copies of the row data on every update, which is not efficient, and currently doesn't function really well anyway. It would be nice if there were a cheaper way to tell the component to check for change. It would also be nice if this components state information were not stored in the applications input rows, so that when a new array is given, the state information is not lost.

Caballerog commented 7 years ago

+1 @shattar could be interesting a configuration option to decide the strategy to render. I think that a table could be update in a lot of the real cases.

amcdnl commented 7 years ago

@shattar - What API would you propose?

Heres how I would handle that, it doesn't seem that bad...

onBlur({ rowIndex, value }) {
   let rows = [...this.rows];
   rows[rowIndex] = value;
   this.rows = rows;
}

If you look at any other React based implementation such as Facebooks Fixed Datatable you are gonna find similar APIs. http://facebook.github.io/fixed-data-table/

shattar commented 7 years ago

@amcdnl - What do you think about exposing a markForCheck function so that parent components can mark it when they mutate the array in place? See here... https://plnkr.co/edit/hsBGsgUmEfh29O1vEz4W?p=preview . I like this option because it seems pretty clean and follows ng2 semantics. It would allow the desperate person to get the job done without compromising the purity of the existing 'array of rows' API.

Or (less desirable IMO) make a 'rows object' option, { rows: any[] }, where the shallow copy is extremely small this.data = Object.assign({}, this.data); instead of the potentially large shallow copy let rows = [...this.rows];. See here... https://plnkr.co/edit/HLzuNYJ0rqOnZxPBssx5?p=preview . The component could use the array of rows input like it does now as the primary data source, and check the new rows object as the secondary data source if the primary data source is null. This way people don't have to mess with the rows object interface if they don't need it.

shattar commented 7 years ago

@amcdnl - I have to mention... I do think that trying to get around an array shallow copy in the name of efficiency is a bit of premature optimization. I would much rather have the other problem addressed. The issue where, if you do give a new rows array, the selection and sort order are messed up because the state information was stuck in the 'old' array.

amcdnl commented 7 years ago

Ya, I'm going to reach out to others in community and see what their thoughts on this problem is. I think the main issue is that in a few places the table manipulates the inner state without propagating upward which violates the paradigm, however, removing that would remove features like column orders/sorting/etc.

I'm good w/ exposing that until we find a better approach. Could you PR that please?

derchirurg commented 7 years ago

I think this is a lot of "overhead" with copying the whole array all the times I do an update. This is not affordable for my usecase.

elvisbegovic commented 7 years ago

Mee to edit and remove srent percutef aitomaticslly of its onpush strategy please set change detection ...

amcdnl commented 7 years ago

Added a refresh function in 1.4.0

elvisbegovic commented 7 years ago

@amcdnl from my class I use @ViewChild('datatable') datatable; then I refresh using this.datatable.refresh() after delete row but ... nothing's happend ... it doesn't refresh, why ?

PS: in fact when I delete or edit and after that if I resize window or scroll inside datatable a litle bit ...my data are updated !! why this behavior , here's my gif

refresh data

Caballerog commented 7 years ago

This night (local time) I going to test the new feature too.

mashaalmemon commented 7 years ago

Experiencing the same issue. I hit the refresh() function, the table is not refreshed.

germanAd commented 7 years ago

This problem still exist? In version 0.9.3 this bug not hepend

elvisbegovic commented 7 years ago

@germanAd 0.9.3 was good for me too but we are on 1.4.0 ... yes problem still here

Caballerog commented 7 years ago

In my case doesn't work :(.

This is my code:

TS:

private vars...

  @ViewChild('datatableTeaching') table;

more code...

  newRow(){
  this.rows.unshift({
      price: 100,
      duration: 60,
      state: 1,
     });
    this.table.refresh()
}

HTML:

 <datatable #datatableTeaching class="material" 
            [rows]="rows" 
            [columnMode]="'force'"
            [headerHeight]="50"
            [rowHeight]="'auto'"
            [footerHeight]="'auto'"
            >
....
</datatable>
germanAd commented 7 years ago

Why need add refresh function ?need compare with version(0.9.3) that refresh worked automatically and fixed this

elvisbegovic commented 7 years ago

another technology (onpush strategy detection change) check it http://blog.thoughtram.io/angular/2016/02/22/angular-2-change-detection-explained.html please read all this issue link is above

Caballerog commented 7 years ago

@germanAd the module changed the strategy because of perform when the table is enormous.

germanAd commented 7 years ago

@Caballerog Have resolve this problem?

elvisbegovic commented 7 years ago

@germanAd dude where you read "resolve this problem"

You asked "why need this refresh function, because in 0.9.3 it worked automatically?"

so about this question : ME and @Caballerog tell you it's because of code refactoring, NOW we have OnPush change detection strategy.

Again please read this issue from the begining and open link I posted to understand better

thanks

Caballerog commented 7 years ago

@germanAd no, I yet didn't resolve my problem. @istiti told you what is the current problem.

karmis commented 7 years ago

I have this problem too. any ideas ?

ldanielduarte commented 7 years ago

+1 refresh() is not working properly. Table rows are updated after resizing.

ldanielduarte commented 7 years ago

Label says is "Working as Intended". Should the community expect updates on this issue?

karmis commented 7 years ago

My solution ... (its bad, but its working)

var self = this;
setTimeout(function () {
    self.mediaTable.resize.emit(self.tableColumns[0], self.tableColumns[0].width);
}, 0);
amcdnl commented 7 years ago

Working on a better solution for this. Added a custom trackBy function per @robwormald suggestion. Still not fully working yet though.

elvisbegovic commented 7 years ago

highly waiting this feature cause can't update properly my riws actually !

shattar commented 7 years ago

I can work on this sometime within the next couple of days.

elvisbegovic commented 7 years ago

@amcdnl can you please explain how delete and edit row today? 1.7.0 because before was very simple: delete = this.dataFiles.splice(this.selectedFiles.$$index, 1) update = this.dataFiles[0].nameFile = 'hmm';

how can achieve this actually ? it doesn't work even if I do this:

let rows = [...this.dataFiles];
rows[0].nameFile = 'hmm';
this.dataFiles = rows;

really lost here; have you example, my app is blocking and i'm still in 0.9 to get it working if not this version big problems lol thanks

amcdnl commented 7 years ago

My inline editing example seems to work. Try using the custom track by too.

elvisbegovic commented 7 years ago

@amcdnl what do you mean by custom track by too ? I just check your inline edit and I can see simply: this.rows[row.$$index][cell] = event.target.value;

Can I edit my row like this? I don't understand

Rezi commented 7 years ago

Even the inline example does not work. Here http://swimlane.github.io/angular2-data-table/# when I go to "inline editing" demo and dobuble "Cora Chase" then change it. Nothing happen after clicking out. Then when you click the second line "Ethel Price" it gets value of modified "Cora Chase". This I don't call working example. Sorry

ipassynk commented 7 years ago

I am trying to upgrade from 0.10.1 to 1.7.0 and I can't make the following functions work (they worked in 0.10.1)

  1. add/remove row in a grid without refreshing the whole data set.
  2. live update is not working. I also checked the live update demo and a cell value is get updated ONLY when I click on it.
motcowley commented 7 years ago

Just been trying to get the updating working also. Just as a follow on from what @karmis suggested, the following (bad) solution works for me (1.7.0) :

@ViewChild('datatableStats') table;

...some code...

removeRow() {
this.rows.pop();
setTimeout(() => {
            this.table.resize.emit();
        }, 0);
}

Tried adding: [trackByProp]="'name'"

But this didn't seem to do anything.

elvisbegovic commented 7 years ago

@motcowley this is very tricky ! I think @amcdnl is workin on this because actually everbody have this problem!

hope it will be better asap

ipassynk commented 7 years ago

I tried table.resize.emit() and it does not work for me. Can you please explain why it should work?

So instead of calling resize.emit() I added:

 window.dispatchEvent(new Event('resize')); 

after remove/add and it works for me now.

amcdnl commented 7 years ago

Ya, I'm not sure on best approach. I've spoke w/ quite a few ppl on the matter, I think a custom ngFor is the approach I might have to take.

I'm on vaca this week so won't have much to add right now.

elvisbegovic commented 7 years ago

@ipassynk but don't works when edit a row ? for me your solution works only id remove row

ipassynk commented 7 years ago

@istiti - window resize helps only with add/remove row but not edit. Cell click helps with edit. @amcdnl - thank you very much for creating and supporting this library.

elvisbegovic commented 7 years ago

@ipassynk yep you are right, it's very big dammage this regression doesn't allow me use this table now... I really need edit rows but no issue

ipassynk commented 7 years ago

the same - I need editing as well. I use this hack for now - $('.datatable-body-cell').trigger('click'); after edit.

amcdnl commented 7 years ago

I posted a question on stack overflow. Still investigating.

Caballerog commented 7 years ago

Hi @amcdnl , you're a great boy! (There are already answers from stackoverflow!)

amcdnl commented 7 years ago

So, after chatting w/ a few cool cats...I'm gonna remove OnPush from the top level datatable. Everything else will stay onpush for good perf though. I'm working on this now, so this will be done shortly. Thanks for your patience.

elvisbegovic commented 7 years ago

@amcdnl thanks I could update to last version because actually i'm stil on 0.9 :) for this feature (remove/add/edit/ thanks

Caballerog commented 7 years ago

Thanks @amcdln, I going to testing your new feature in my proyect when it's ready ;-) ᐧ


Dr. Carlos Caballero González PTFP Sistemas y Aplicaciones Informáticas - Junta de Andalucía LinkedIn: http://lnkd.in/xVG3qV Twitter: http://www.twitter.com/carlilloGitHub: http://www.github.com/caballerog------------------------------------------------

2016-12-01 7:12 GMT+01:00 vltr notifications@github.com:

@amcdnl https://github.com/amcdnl thanks I could update to last version because actually i'm stil on 0.9 :) for this feature (remove/add/edit/ thansk

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/swimlane/angular2-data-table/issues/255#issuecomment-264087697, or mute the thread https://github.com/notifications/unsubscribe-auth/AIZj36RDKLDG-jB5oZXb5tE3FgNpL1CVks5rDmVPgaJpZM4KqbVB .