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.

Sign up for GitHub

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

Jump to bottom

feat(stark-ui): add footer to table component #1601

Merged
merged 1 commit into from
Feb 6, 2020
Merged

feat(stark-ui): add footer to table component #1601

merged 1 commit into from
Feb 6, 2020

Conversation

nicanac
Copy link
Contributor

@nicanac nicanac commented Jan 30, 2020

ISSUES CLOSED: #1540

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

Implement a footer for the table component

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1540

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@nicanac nicanac added this to the 10.0.0-rc.4 milestone Jan 30, 2020
@nicanac nicanac added this to In progress in 10.0.0-rc.4 via automation Jan 30, 2020
@coveralls
Copy link

coveralls commented Jan 31, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 94.102% when pulling 159b245 on nicanac:feature/ui-table-add-footer into 172aaf2 on NationalBankBelgium:master.

10.0.0-rc.4 automation moved this from In progress to Review in progress Feb 3, 2020
christophercr
christophercr suggested changes Feb 4, 2020
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

Some remarks... let me know what you think?

packages/stark-ui/src/modules/table/entities/column-properties.intf.ts Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/column.component.ts Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Show resolved Hide resolved
christophercr
christophercr reviewed Feb 4, 2020
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

Small remarks

packages/stark-ui/src/modules/table/components/column.component.html Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Outdated Show resolved Hide resolved
christophercr
christophercr reviewed Feb 5, 2020
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

See my comments...

packages/stark-ui/src/modules/table/components/column.component.html Outdated Show resolved Hide resolved
packages/stark-ui/src/modules/table/components/table.component.ts Outdated Show resolved Hide resolved
christophercr
christophercr reviewed Feb 5, 2020
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! It is working now! Just small remarks and also... don't forget to write unit tests for this new feature 😉

showcase/src/assets/examples/table/with-footer/table.html Outdated Show resolved Hide resolved
showcase/src/assets/examples/table/with-footer/table.ts Outdated
selector: "showcase-table-with-footer",
templateUrl: "./table-with-footer.component.html",
styleUrls: ["./table-with-footer.component.scss"],
/* tslint:disable-next-line:use-view-encapsulation */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this line as it is not relevant for the example code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! It is working now! Just small remarks and also... don't forget to write unit tests for this new feature 😉

Yes sure, I'll create an issue for this purpose : #1613

christophercr
christophercr approved these changes Feb 5, 2020
View reviewed changes
Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

LGTM

SuperITMan
SuperITMan reviewed Feb 5, 2020
View reviewed changes
packages/stark-ui/src/modules/table/components/column.component.html Outdated
@@ -39,4 +39,9 @@
*ngTemplateOutlet="columnTemplate; context: { $implicit: { rowData: rowItem, displayedValue: getDisplayedValue(rowItem) } }"
></ng-container>
</td>
<ng-container *ngIf="footerValue !== 'undefined'">
<td mat-footer-cell *matFooterCellDef>
{{ footerValue }}
Copy link
Member

Choose a reason for hiding this comment

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

@christophercr @nicanac What do you think of adding the pipe | translate here ?
Like this, it's easy for the developer to provide a translation key.

Otherwise, the dev will have to play with translateService.instant("TRANSLATION_KEY") in the component using the Stark Table...

Copy link
Contributor Author

@nicanac nicanac Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@SuperITMan , @christophercr ,mhhh why not, but if we pass a number, isn't it an issue ?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm... You're right! I thought the pipe was able to get a string but it's not the case.
What we could do to make this working is:

<td mat-footer-cell *matFooterCellDef>
    {{ footerValue!.toString() | translate }}
</td>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oki ok it's implemented.

showcase/src/app/demo-ui/components/table-with-footer/table-with-footer.component.ts Outdated
};

public columns: StarkTableColumnProperties[] = [
{ name: "id", label: "Id", footerValue: "Total" },
Copy link
Member

Choose a reason for hiding this comment

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

If we implement the pipe | translate, we should provide the translation key instead of Total 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

SuperITMan
SuperITMan approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@SuperITMan SuperITMan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

10.0.0-rc.4 automation moved this from Review in progress to Reviewer approved Feb 6, 2020
@SuperITMan SuperITMan merged commit 2f03477 into NationalBankBelgium:master Feb 6, 2020
10.0.0-rc.4 automation moved this from Reviewer approved to Done Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophercr christophercr christophercr approved these changes

@SuperITMan SuperITMan SuperITMan approved these changes

Assignees
No one assigned
Labels
comp: stark-ui type: feature
Projects
No open projects
10.0.0-rc.4
  
Done
Milestone
10.0.0-rc.4
Development

Successfully merging this pull request may close these issues.

ui: table implement footer
4 participants
@nicanac @coveralls @christophercr @SuperITMan