search

niiknow / vue-datatables-net

Vue jQuery DataTables.net wrapper component
https://niiknow.github.io/vue-datatables-net/
MIT License
171 stars 58 forks source link

allow pass array of fields #10

Closed xPlux closed 5 years ago

noogen commented 5 years ago

I prefer not to use array as it may introduce unnecessary complexity.

xPlux commented 5 years ago

So, if you allow to pass fields only as object then I can't declare array like this: fields: [{name: 'foo', label: 'Foo'}] and i have to do this: fields: {'foo': {label: 'Foo'}} but in this case name parameter is unnecessary and line 158 in VdtnetTable.vue field.name = field.name || k does not make sense. Yes, I can overwrite name but why?

noogen commented 5 years ago

Yes. To give several examples of using array in this patch that may cause downstream issues:

  1. The for loop (1a) and code for field name (1b) need updating to handle arrays: https://github.com/niiknow/vue-datatables-net/blob/b2f11ba9ab42fce3f5c7f443f0e6a910900424bf/src/VdtnetTable.vue#L156 (1a) and https://github.com/niiknow/vue-datatables-net/blob/b2f11ba9ab42fce3f5c7f443f0e6a910900424bf/src/VdtnetTable.vue#L158 (1b)

Explain: https://stackoverflow.com/questions/500504/why-is-using-for-in-with-array-iteration-a-bad-idea

  1. In jQuery Datatables, name is not required but it is required in my component. Why is that? I'm using name here:

I know in jQuery Datatables, you can define columns as [{columnDef1}, null, null, {columnDef2}, null]. If user start to pass this in, we have to handle null. How will we handle defaults? Will these null columns be handle on the server-side? isLocal, sortable, searchable...

Sorry, don't take it the hard way as I don't mean to be offensive. I just want to discuss this a little more. It's all part of code review. I'm suggesting that maybe this pull request is not complete? It need to provide strategy, at a minimum, to handle the situations I for-see above and have unit tests to prove?

For example, if name is not provided, it will default to using 'column-[1-n]' for slot name.