BccPagination #300
BccPagination #300
Conversation
starkris51
force-pushed
the
BccPagination
branch
from
9cd84d1 to
5a76ff7
Compare
|
@laurensgroeneveld Kunne du sjekke denne? |
There was a problem hiding this comment.
Thanks a lot, this is an important component currently missing from the library! I have quite a few comments but most of them are pretty small code quality/design things. Functionality wise the component is already very good 👍 Let me know if you don't agree with something 😊
design-library/src/components/BccPagination/BccPagination.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think this file was included by accident - we added some tools for rendering the shadows in #285 but are waiting on updated shadows I think before including them in the design library. In any case it would belong in a different PR.
There was a problem hiding this comment.
The file seems to be still here, despite Github saying this comment is "outdated".
| .bcc-pagination-button-left { | ||
| @apply rounded-l-lg px-3.5 text-interactive; | ||
| } | ||
|
|
||
| .bcc-pagination-button-right { | ||
| @apply rounded-r-lg px-3.5 text-interactive; | ||
| } |
There was a problem hiding this comment.
Same comment on consolidating these in one bcc-pagination-button (or just call them bcc-pagination-arrow?) and then a modifier class that only changes the rounding.
There was a problem hiding this comment.
when I fixed the double border, the right arrow needed different tailwind code and its so little shared code that I don't think its worth having a separate class
| import BccFormLabel from "../BccFormLabel/BccFormLabel.vue"; | ||
|
|
||
| type Props = { | ||
| tableItems: Item[]; |
There was a problem hiding this comment.
For discussion: I'm not sure if tableItems needs to be part of this component. Instead, I feel like we should build pagination into the table component, so that that is the only component that is concerned about the actual data (this doesn't have to happen necessarily in this PR though).
It's still good to have this as a separate component, but instead I feel like it should only concern itself with the actual pagination. That way it can be used for other usecases as well, not just together with our table component. Instead of passing a full tableItems array, we could maybe just pass the amount of items so the amount of pages can be correctly calculated?
Curious to hear your opinion on this!
There was a problem hiding this comment.
i do agree that it should not only be used for the table, the change I've did is that it supports array with any types.
There was a problem hiding this comment.
@laurensgroeneveld i have made a alternative version of this using Radix vue libary, it uses passing amount of items instead of a array.
#301
There was a problem hiding this comment.
@laurensgroeneveld i have made a alternative version of this using Radix vue libary, it uses passing amount of items instead of a array.
Yes, that's what I meant here as well. I'll take a look.
i do agree that it should not only be used for the table, the change I've did is that it supports array with any types.
I feel like the pagination component should not need to be aware of the actual items at all, but instead just the total amount.
There was a problem hiding this comment.
The CSS is much more consolidated, looks really good 👍 Thanks for picking up all the comment, sorry for late re-review 😅 Just have three small comments (two I added as a comment to previous open threads), other than that it looks good. If you can just remove that shadows file I'll approve it 😊
| 'bcc-pagination-left': | ||
| (align === 'left' && displayRowsPerPage === true) || | ||
| (align === 'right' && displayRowsPerPage === false), | ||
| 'bcc-pagination-center': align === 'center', | ||
| 'bcc-pagination': align === 'right', |
There was a problem hiding this comment.
Since the underlying CSS is mostly the same, I think it makes more sense to have bcc-pagination as a default class and then just adding the -left and -center variants in addition when applicable. That's how it's done in most other components as well, for example the secondary button is bcc-button bcc-button-secondary, with the last class just adding or overriding classes that are different from the default.
| watchEffect(() => { | ||
| emit("update:paginatedItems", paginatedRows.value); | ||
| emit("update:totalPages", totalPages.value); | ||
| }); |
There was a problem hiding this comment.
Nice, I should use watchEffect more instead of plain watch, it works nicer for instances like this 👍
|
@laurensgroeneveld i'm wondering about if the Table in pagination storybook should be kept or removed like with the Radix Vue version where i only show the component itself. |
I think that's fine, then when we add pagination support to the Table component itself it can instead be shown there. I didn't add a comment on it before, but you could even add a few stories showing some different options at a glance, like if you hide the page selector, alignment etc. |
I've moved the table as a story instead. |
Change summary
Added BccPagination
Change type
Closes #ISSUE_NUMBER or Part of #ISSUE_NUMBER