-
Notifications
You must be signed in to change notification settings - Fork 490
Add ngPipeArgs to ngPipe
property
#1642
Add ngPipeArgs to ngPipe
property
#1642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good start for PR! Just needs a handful of changes.
expect(testsArray.map(index => { | ||
const dataRow = rows[index]; | ||
|
||
let pipeInstance = app.pipeCurrencyInstance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be const
as we don't re-declare pipeInstance
.
|
||
const NumberFromData = (instance.row(dataRow).data() as Person).id; | ||
const NumberFromTable = $('td:nth-child(1)', dataRow).text(); | ||
console.log(NumberFromData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove these, I assume?
src/angular-datatables.directive.ts
Outdated
@@ -106,13 +106,14 @@ export class DataTableDirective implements OnDestroy, OnInit { | |||
const colsWithPipe = columns.filter(x => x.ngPipeInstance && !x.ngTemplateRef); | |||
colsWithPipe.forEach(el => { | |||
const pipe = el.ngPipeInstance; | |||
const pipeArgs = el.ngPipeArgs ? el.ngPipeArgs : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to el.ngPipeArgs || [];
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "angular-datatables", | |||
"version": "13.0.1", | |||
"version": "13.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this. I'll update the version at a later date.
@@ -4,9 +4,9 @@ You need to install its dependencies: | |||
|
|||
```bash | |||
# JS file | |||
npm install datatables.net-colreorder --save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move "responsive installation fix" to a different PR? We try not to mix unrelated code changes in single PR.
ngPipe
property
7eb468d
to
11eb31b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
So,I changed everything you told me :
- Remove version 13.1.0
- Remove responsive installation part
- Remove console.log() (Yes it was a mistake 😅)
- Use
||
instead of? :
- Replace
let
byconst
For me, I think it's all good !
Regards,
Adrien.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Hi,
I added the
ngPipeArgs
option with a test indemo/src/app/advanced/using-ng-pipe.component.spec.ts
file. I chose to take the data ID and convert it to currency because I didn't know how to import data.I also saw that the responsive installation was wrong, I decided to fix it.
Finally, this is my first pull request, if I did something wrong, don't hesitate to tell me.
Regards,
Adrien.