search

Juicy / juicy-table-repeat

A workaround custom element which allows you to use dom-repeat inside table under IE and FF
MIT License
4 stars 1 forks source link

Array notifications are applied wrong after inserting and removing a row #5

Open warpech opened 8 years ago

warpech commented 8 years ago

Steps to reproduce:

  1. Click on "Insert a row"
  2. Click on "Update first row"
  3. Click on "Remove first row"
  4. Click on "Update first row"

The last 3 rows are updated with a new price. Only one row (the first row) should be updated.

See:

screen

miyconst commented 8 years ago

@tomalec could you please take a look?

tomalec commented 8 years ago

Shorted test case:

  1. Click on "Update first row"
  2. Click on "Remove first row"
  3. Click on "Update first row"
tomalec commented 8 years ago

It is index .0 vs key #0 mismatch, probably due to the fact that we are re-stamping all the rows at https://github.com/Juicy/juicy-table-repeat/blob/gh-pages/juicy-table-repeat.html#L77 in case of remove/insert of just one.

tomalec commented 8 years ago

It seems we are facing all the problems that are solved by Polymer's dom-repeat so we would probably need to move most of its logic to here.

@warpech, @miyconst WDYT about forking (and making a PR of) dom-repeat and implementing what was proposed at https://github.com/Polymer/polymer/issues/3448?

[edit: I have removed my proposition as it was simply wrong]

For me injecting one more template to the dom-repeat seems easier that replicating its behavior. Also to replicate dom-repeat logic, we would need to go through and understand entire code anyway.

tomalec commented 8 years ago

Or maybe we should just ask people to fallback to:

<juicy-table>
<template is="dom-repeat">
</template>
</juicy-table>

na make juicy-element do nothing more than just extending HTMLTableElement ?

warpech commented 8 years ago 
<juicy-table>
<template is="dom-repeat">
</template>
</juicy-table>

If it works, I'd love it!

miyconst commented 8 years ago

@tomalec am I right that you want to stamp <tr>s with native Polymer's dom-repeat and them move them into <table>?

Well, if that would work, I am all for it.

P.S. Any solution which works in IE to generate table rows is good for me, even if it requires a bit more code or another <template>.

warpech commented 8 years ago

I wonder if you can get this to work:

<juicy-table class="table"><!-- "table" is a Bootstrap class -->
  <thead>
    <tr>
      <template is="dom-repeat" items="{{Headers}}"><!-- my column headers -->
        <th>{{item.Label}}</th>
      </template>
    </tr>
  </thead>
  <tbody>
    <template is="dom-repeat" items="{{People}}"><!-- my rows -->
      <tr><td>{{item.FirstName}}</td><td>{{item.LastName}}</td></tr>
    </template>
  </tbody>
</juicy-table>

See source of Bootstrap .table

tomalec commented 8 years ago

@warpech

If it works, I'd love it!

It works but you loose user agent styles

See https://jsbin.com/coxere/edit?html,output

@miyconst

am I right that you want to stamp s with native Polymer's dom-repeat and them move them into

? Not really. Anyway I need to review my proposition in Polymer's issue as it seems not to solve the problem.

miyconst commented 8 years ago

@warpech as far as I remember it won't work. IE removes every node from <tbody> and <table> which is not <tr> and puts it after the element.

So, it will be equal to:

<juicy-table class="table"><!-- "table" is a Bootstrap class -->
  <thead>
    <tr>
    </tr>
  </thead>
  <template is="dom-repeat" items="{{Headers}}"><!-- my column headers -->
    <th>{{item.Label}}</th>
  </template>
  <tbody>
  </tbody>
  <template is="dom-repeat" items="{{People}}"><!-- my rows -->
    <tr><td>{{item.FirstName}}</td><td>{{item.LastName}}</td></tr>
  </template>
</juicy-table>

Does not seem to work even in Chrome - https://jsfiddle.net/qc5qv5e5/.

tomalec commented 8 years ago

For that we will need juicy-tr, juicy-thead,...

miyconst commented 8 years ago

@tomalec your solution does not seem to work in IE:

image

tomalec commented 8 years ago

My bad. You'r right, I was checking in Edge :(

So we need to either

  1. fix this bug. I'm afraid that this and bugs that are to come, will require us to do more or less this: https://github.com/Polymer/polymer/blob/master/src/lib/template/dom-repeat.html, so in long term it should be easier to..
  2. Implement (few weeks) / wait for Polymer to implement (∞ ?) https://github.com/Polymer/polymer/issues/3448#issuecomment-243159847
tomalec commented 8 years ago

@warpech how urgently do we need a fix, and how crucial it is?

miyconst commented 8 years ago

@warpech, @tomalec what if we go the "smart" way and just drop the IE support?

tomalec commented 8 years ago

what if we go the "smart" way and just drop the IE support?

I'd love to :sunglasses: Especially, that it seems that event Microsoft is pushing Edge forward and do not bother much about IE11 support.

The question is: what about our customers, and demos that are supposed to look clean and work cross platform?

warpech commented 8 years ago

No, it is not urgent. I just saw it and reported it for reference.

Let's not drop IE11support before Polymer.

miyconst commented 8 years ago

Let's not drop IE11support before Polymer.

Polymer only partially supports it.

warpech commented 8 years ago

Let's wait until Polymer 2.0, which may fix this

tomalec commented 8 years ago

https://github.com/Starcounter/CompanyTrack/issues/233

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.