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

feat(stark-ui): table - Add support to show rows counter #1285

Conversation

SuperITMan
Copy link
Member

@SuperITMan SuperITMan commented May 19, 2019

ISSUES CLOSED: #1244

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] 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?

There is no rows counter available in the table component.
It was the case in the previous implementation of Stark.

Issue Number: #1244

What is the new behavior?

Provide optional rows counter in the table component.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@christophercr I hesitate about the number to show...
Here, I display {{ data.length }}, as it was the case in the previous implementation, but we could display {{ dataSource.data.length }} which represents the displayed rows (in case of filtering).

What do you think we should use ?

(I have the feeling we should display {{ dataSource.data.length }})

@SuperITMan SuperITMan added this to the 10.0.0-beta.8 milestone May 19, 2019
@SuperITMan SuperITMan requested a review from christophercr May 19, 2019 14:04
@SuperITMan SuperITMan force-pushed the feature/table-add-total-items branch from 8b7c7b4 to 7612988 Compare May 19, 2019 14:09
@SuperITMan SuperITMan requested a review from carlo-nomes May 20, 2019 11:19
@SuperITMan SuperITMan force-pushed the feature/table-add-total-items branch from 7612988 to fdaba0d Compare May 20, 2019 11:51
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. There are also a couple of layout issues:

1.- when the showRowsCounter is used together with customTableActionsType="alt":
rows counter and alternative actions

2.- when the showRowsCounter is used in a table with transcluded action bar:
rows counter and transcluded action bar

@christophercr
Copy link
Collaborator

christophercr commented May 20, 2019

About the number to show, I also think we should show the number of displayed rows. In fact, we are already taking that into account to update the total pages in the pagination...

@coveralls
Copy link

coveralls commented May 20, 2019

Coverage Status

Coverage increased (+0.008%) to 92.821% when pulling bdad94f on SuperITMan:feature/table-add-total-items into 629455d on NationalBankBelgium:master.

@SuperITMan SuperITMan force-pushed the feature/table-add-total-items branch 2 times, most recently from 0d983a6 to 351f428 Compare May 21, 2019 06:53
@SuperITMan
Copy link
Member Author

@cnomes @christophercr I updated my PR. Could you please have a look ? 😊

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.

One small remark.

Another thing although not related to this PR. Could you adapt the example of the "Table with fixed header" to pass a pagination config with itemsPerPage: 10? Otherwise the table is too small so the scroll bars are not shown which makes the "fixed header" feature not to be showcased :(

.stark-table-rows-counter {
flex: 1;
text-align: center;
line-height: 35px;
Copy link
Collaborator

@christophercr christophercr May 21, 2019

Choose a reason for hiding this comment

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

In my opinion this line-height could be lower, maybe 22px?
rows counter small screens 2

Cause right now with 35px it looks a bit weird in small screens:
rows counter small screens

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep', it's OK for me. I took the value we had before.

@SuperITMan SuperITMan force-pushed the feature/table-add-total-items branch from 351f428 to bdad94f Compare May 21, 2019 14:00
@SuperITMan
Copy link
Member Author

@christophercr PR updated 😊

@christophercr christophercr merged commit f3e0c40 into NationalBankBelgium:master May 21, 2019
@SuperITMan SuperITMan deleted the feature/table-add-total-items branch August 8, 2019 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: table - add total number of items
3 participants