Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
warpech opened this issue Aug 25, 2016 · 20 comments
Open

Comments

@warpech
Copy link
Contributor

warpech commented Aug 25, 2016

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

@warpech warpech added the bug label Aug 25, 2016
@miyconst
Copy link

@tomalec could you please take a look?

@tomalec tomalec self-assigned this Aug 29, 2016
@tomalec
Copy link
Member

tomalec commented Aug 29, 2016

Shorted test case:

  1. Click on "Update first row"
  2. Click on "Remove first row"
  3. Click on "Update first row"

@tomalec
Copy link
Member

tomalec commented Aug 29, 2016

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
Copy link
Member

tomalec commented Aug 29, 2016

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 Polymer/polymer#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
Copy link
Member

tomalec commented Aug 29, 2016

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
Copy link
Contributor Author

warpech commented Aug 29, 2016

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

If it works, I'd love it!

@miyconst
Copy link

@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
Copy link
Contributor Author

warpech commented Aug 29, 2016

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
Copy link
Member

tomalec commented Aug 29, 2016

@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
Copy link

miyconst commented Aug 29, 2016

@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
Copy link
Member

tomalec commented Aug 29, 2016

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

@miyconst
Copy link

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

image

@tomalec
Copy link
Member

tomalec commented Aug 29, 2016

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 (∞ ?) Add a way for developers to specify a 'container' element within a 'dom-repeat' template Polymer/polymer#3448 (comment)

@tomalec
Copy link
Member

tomalec commented Aug 29, 2016

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

@miyconst
Copy link

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

@tomalec
Copy link
Member

tomalec commented Aug 29, 2016

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

I'd love to 😎 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
Copy link
Contributor Author

warpech commented Aug 29, 2016 via email

@miyconst
Copy link

Let's not drop IE11support before Polymer.

Polymer only partially supports it.

@warpech
Copy link
Contributor Author

warpech commented Sep 26, 2016

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

@tomalec
Copy link
Member

tomalec commented Sep 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants