spatie / vue-table-component

A straight to the point Vue component to display tables
http://vue-table-component.spatie.be
MIT License
588 stars 149 forks source link

update vue js to version 2.5 add required keys to v-for #129

Closed euqip closed 6 years ago

euqip commented 6 years ago

Happy to find a simple table component. Thanks for that great work. Tried it and found somme errors while I had Vuejs@2.5 globally installed. So I tried to get them out. In the same time I had to add the records IDs or keys.

euqip commented 6 years ago

Hi Sebastian,

When using vuejs 2.5, there is a need to have a key, otherwise at runtime, vue produces a warning.

There were no key in the data table, so I added one, the id field, and made it invisible. I haven't seen any other field able to be used as unique key.

When applying this change, there is no warning anymore.

Thanks to have checked it.

Benoit

@sebastiandedeyne requested changes on this pull request.


In docs/index.html https://github.com/spatie/vue-table-component/pull/129#discussion_r170543377 :

         ]"

sort-by="songs" sort-order="asc"

I don't see why this is necessary?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spatie/vue-table-component/pull/129#pullrequestreview-99242365, or mute the thread https://github.com/notifications/unsubscribe-auth/AECGSSQk-uTT6GHHVEhmAUJ_5ZkKBJrrks5tYoKUgaJpZM4SK7-O .

euqip commented 6 years ago

Hi Sebastian,

I inserted a key because i saw the warning, but you are right the key in the data is not necessary, it is already defined when using the table-row component where an internal Key is resulting from a method inside of tablecomponent.vue. I inserted a key for the pagination and did the same for the data, where it was unneeded.

<table-row
v-for="row in displayedRows"
:key="row.vueTableComponentInternalRowId"
:row="row"
:columns="columns"
                        @rowClick="emitRowClick"
></table-row>

The internal RowId is not incompatible with the use of a data index field.

I suggest you refuse this PR. This PR covers the slot-scope warning too. I can make one PR just to cover this point

Regards

Benoit 2018-03-02 10:42 GMT+01:00 Sebastian De Deyne notifications@github.com:

@sebastiandedeyne requested changes on this pull request.

In package.json https://github.com/spatie/vue-table-component/pull/129#discussion_r171801442 :

@@ -67,5 +67,6 @@ "^.+\.js$": "/node_modules/babel-jest", ".*\.(vue)$": "/node_modules/jest-vue-preprocessor" }

  • }
  • },
  • "dependencies": {}

This doesn't belong here

In docs/index.html https://github.com/spatie/vue-table-component/pull/129#discussion_r171801991 :

@@ -133,10 +133,10 @@

<table-component :data="[

  • { firstName: 'John', lastName: 'Lennon', instrument: 'Guitar', birthday: '04/10/1940', songs: 72 },
  • { firstName: 'Paul', lastName: 'McCartney', instrument: 'Bass', birthday: '18/06/1942', songs: 70 },
  • { firstName: 'George', lastName: 'Harrison', instrument: 'Guitar', birthday: '25/02/1943', songs: 22 },
  • { firstName: 'Ringo', lastName: 'Starr', instrument: 'Drums', birthday: '07/07/1940', songs: 2 },
  • { key: 1, firstName: 'John', lastName: 'Lennon', instrument: 'Guitar', birthday: '04/10/1940', songs: 72 },

Sorry, I still don't understand what this does. When I run the example locally, I'm not getting any warnings.

I understand why key is necessary and when it gives warnings. Adding it to the li in pagination makes perfect sense, since Vue throws a warning when iterated elements don't have a key. However, this has nothing to do with the contents of an object. I don't understand why adding a key property to an object would solve anything, key is for elements, not data.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/spatie/vue-table-component/pull/129#pullrequestreview-100718905, or mute the thread https://github.com/notifications/unsubscribe-auth/AECGSThi6d1H6xRiSTlXDO8muOBUwr9Wks5taRQCgaJpZM4SK7-O .

sebastiandedeyne commented 6 years ago

Ok, I made the changes myself on master, thanks!