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

Add ngPipeArgs to ngPipe property #1642

Merged

Conversation

adrien-syrotnik
Copy link
Contributor

Hi,

I added the ngPipeArgs option with a test in demo/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.

Copy link
Collaborator

@shanmukhateja shanmukhateja left a 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;
Copy link
Collaborator

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)
Copy link
Collaborator

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?

@@ -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 : [];
Copy link
Collaborator

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",
Copy link
Collaborator

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

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.

@shanmukhateja shanmukhateja changed the title Add ngPipeArgs and correct responsive installation Add ngPipeArgs to ngPipe property Feb 26, 2022
Copy link
Contributor Author

@adrien-syrotnik adrien-syrotnik left a 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 by const

For me, I think it's all good !

Regards,
Adrien.

Copy link
Collaborator

@shanmukhateja shanmukhateja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@shanmukhateja shanmukhateja merged commit 0d0c379 into l-lin:master Mar 1, 2022
@shanmukhateja shanmukhateja mentioned this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants