Feat(OnyxPagination): implement inline pagination #4310
Conversation
🦋 Changeset detectedLatest commit: 2f21374 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This is an auto-generated pull request. All Playwright screenshots have been updated. Co-authored-by: ChristianBusshoff <[email protected]>
There was a problem hiding this comment.
Nice to have: Currently the skeleton is defined to have a fixed with in Figma. Imo it would be nice if the skeleton has the same width as the "default" variant here in the screenshot (considering density) so the layout does not jump around. Should we clarify this with UX?
| /** | ||
| * The display mode of the pagination | ||
| * @default select | ||
| */ | ||
| mode?: OnyxPaginationModes; |
There was a problem hiding this comment.
| /** | |
| * The display mode of the pagination | |
| * @default select | |
| */ | |
| mode?: OnyxPaginationModes; | |
| /** | |
| * The display mode of the pagination | |
| * @default select | |
| */ | |
| type?: PaginationType; |
Would prefer naming it "type" here :) It think thats more aligned with other components + Figma
| "previous": "vorherige Seite", | ||
| "next": "nächste Seite", | ||
| "buttonLabel": "Seite {page}", | ||
| "ellipsis": "Weitere Seiten", |
There was a problem hiding this comment.
| "ellipsis": "Weitere Seiten", | |
| "morePages": "Weitere Seiten", |
Would prefer to name the key after what it describes, not how its displayed visually in the component :)
| <template> | ||
| <OnyxInlinePagination | ||
| v-if="props.mode === 'inline'" | ||
| v-bind="selectPaginationProps" |
There was a problem hiding this comment.
| v-bind="selectPaginationProps" | |
| v-bind="paginationProps" |
We no longer use it only for the select now ;)
There was a problem hiding this comment.
(optional) It might be cleaner to define separate screenshot tests for select and inline so we don't need to use several conditions to use either one setup or the other :)
Also it might be easier this way if we add the "compact" pagination in the future
| > | ||
| <OnyxIcon :icon="iconChevronLeftSmall" /> | ||
| </button> | ||
| <template v-for="pageNumber in displayPagesNumbers" :key="`page-${pageNumber}`"> |
There was a problem hiding this comment.
| <template v-for="pageNumber in displayPagesNumbers" :key="`page-${pageNumber}`"> | |
| <template v-for="pageNumber in displayPagesNumbers" :key="pageNumber"> |
Is there a reason for the custom key?
| :class="['onyx-pagination__inline__ellipsis']" | ||
| :aria-label="t('pagination.ellipsis')" | ||
| > | ||
| <span aria-hidden="true">…</span> |
There was a problem hiding this comment.
| <span aria-hidden="true">…</span> | |
| <span aria-hidden="true">...</span> |
Why don't we use regular dots?
| :aria-label="t('pagination.ellipsis')" | ||
| > | ||
| <span aria-hidden="true">…</span> | ||
| <OnyxVisuallyHidden> |
| border-radius: var(--onyx-radius-sm) 0 0 var(--onyx-radius-sm); | ||
| border-left: var(--onyx-pagination-border-size) solid | ||
| var(--onyx-color-component-border-neutral); | ||
| } | ||
| &:last-of-type { | ||
| border-radius: 0 var(--onyx-radius-sm) var(--onyx-radius-sm) 0; |
There was a problem hiding this comment.
| border-radius: var(--onyx-radius-sm) 0 0 var(--onyx-radius-sm); | |
| border-left: var(--onyx-pagination-border-size) solid | |
| var(--onyx-color-component-border-neutral); | |
| } | |
| &:last-of-type { | |
| border-radius: 0 var(--onyx-radius-sm) var(--onyx-radius-sm) 0; | |
| border-radius: var(--onyx-pagination-border-radius) 0 0 var(--onyx-pagination-border-radius); | |
| border-left: var(--onyx-pagination-border-size) solid | |
| var(--onyx-color-component-border-neutral); | |
| } | |
| &:last-of-type { | |
| border-radius: 0 var(--onyx-pagination-border-radius) var(--onyx-pagination-border-radius) 0; |
Since we use the border radius more than 1 time, I'd prefer to add a new CSS variable for this so its easier to adjust the value in the future
| mode?: OnyxPaginationModes; | ||
| }; | ||
|
|
||
| export type OnyxPaginationModes = "select" | "inline"; |
There was a problem hiding this comment.
Imo we should talk with UX about aligning the property values with Figma. I'd prefer using select and inline instead of modern and classic so we name the types after what they are, not how they might be used. Also thats less opinionated about the usage
2 participants
Relates to #1714
added InlinePagination
added tests
didn't implement Keyboard-control via left, right cause of aria-pattern