feat(PPDSC-2839): pagination support for custom components #707

JohnTParsons · 2023-03-14T09:16:42Z

There are new stories for Pagination input and selection custom components.
They are called "Variations in input": http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2839-custom-pagination/storybook/?path=/docs/components-pagination--story-variations-in-input
and "Variations in selection": http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2839-custom-pagination/storybook/?path=/docs/components-pagination--story-variations-in-selection

Some refactoring and fixing of iteration 1 code was required.

This should now satisfy all possible use cases.

DON'T look at the Documentation Site URL on this ticket. The one on #712 is the most up-to-date.

I have done:

Written unit tests against changes
Written functional tests against the component and/or NewsKit site
Updated relevant documentation

I have tested manually:

The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
The screen reader reads and flows through the elements as expected.
There are no new errors in the browser console coming from this PR.
When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

pp-serviceaccount · 2023-03-14T09:27:40Z

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2839-custom-pagination
Storybook: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/ppdsc-2839-custom-pagination/storybook

src/__tests__/__snapshots__/index.test.ts.snap

mutebg · 2023-03-17T09:21:19Z

src/pagination/__tests__/__snapshots__/pagination.test.tsx.snap

+    <nav
+      aria-label="pagination"
      class="emotion-1"
+      data-testid="pagination-container"
    >
-      <a
-        aria-current="page"
-        aria-label="current page, page 3"
-        class="emotion-2"
-        data-testid="pagination-item"
-        href="https://paginationitem.test"
-      >
-        <svg
-          aria-hidden="true"
-          class="emotion-3 emotion-4"
-          fill="currentColor"
-          focusable="false"
-          viewBox="0 0 24 24"
-          xmlns="http://www.w3.org/2000/svg"
-        >
-          <path
-            d="M0 0h24v24H0z"
-            fill="none"
-          />
-          <path
-            d="M13 7h-2v4H7v2h4v4h2v-4h4v-2h-4V7zm-1-5C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.41 0-8-3.59-8-8s3.59-8 8-8 8 3.59 8 8-3.59 8-8 8z"
-          />
-        </svg>
-      </a>
-    </ul>
-  </nav>
+      <ul
+        class="emotion-2"
+      >
+        <a
+          aria-current="page"
+          aria-label="current page, page 3"
+          class="emotion-3"
+          data-testid="pagination-item"
+          href="https://paginationitem.test"
+        >


NOt sure what's the use case here, but this HTML output seems wrong, a can't be direct child of a UL

Change pushed (it was just a flawed test fixture, not an implementation bug)

src/pagination/components/item/pagination-item.tsx

mutebg · 2023-03-17T11:20:09Z

src/pagination/__tests__/pagination.stories.tsx

+      inputRef.current?.blur();
+    };
+
+    const ariaId = getSSRId();


replace with useReactKeys

Change pushed

mutebg · 2023-03-17T11:21:16Z

src/pagination/__tests__/pagination.stories.tsx

+          <label
+            id={inputLabelId}
+            htmlFor={inputId}
+            aria-hidden
+            style={{display: 'none'}}
+          >
+            Enter page between 1 and {lastPage}
+          </label>


can we use <ScreenReaderOnly as="label"> here?

Change pushed

mutebg · 2023-03-17T11:22:24Z

src/pagination/__tests__/pagination.stories.tsx

@@ -1,4 +1,5 @@
-import React from 'react';
+import React, {useRef} from 'react';
+import {useTheme} from '@emotion/react';


This should be import from newskit

Change pushed

mutebg · 2023-03-17T11:36:58Z

src/pagination/types.ts

    itemButton?: Override<ButtonOrButtonLinkProps & PaginationItemProps>;
+    itemDescription?: Override<TextBlockProps & PaginationItemDescriptionProps>;


What's the difference between those 2? they seem the same to me.
I can do itemButton: (props) => <TextBlock>{props.pageNumber}</TextBlock>
basically, we have 2 ways to replace pagination items.

Can we simplified the API, seems to many options and overrides.

Having a separate itemDescription render prop makes the API simpler for the cases such when the user wants to appends text after the default item Button. Otherwise they would need to manually repeat a lot of code, that is usually hidden behind the scenes, eg

itemButton: (props) => ( <GridLayout columns="auto auto" rows="1fr" autoFlow="row" alignItems="center" columnGap="space010" > <PaginationButton {...props /> <TextBlock>{props.pageNumber}</TextBlock> </GridLayout> )

src/pagination/defaults.ts

mutebg · 2023-03-20T09:14:30Z

src/pagination/__tests__/pagination.stories.tsx

        <PaginationPrevItem />
        <PaginationNextItem />
      </Pagination>
    </StorybookCase>
    <StorybookCase title="Next and previous labels">
-      <Pagination {...defaultProps} aria-label="next and previous labels">
+      <Pagination {...uncontrolledProps} aria-label="next and previous labels">
        <PaginationPrevItem overrides={{width: 'auto'}}>
          Previous


When you pass children shoun't it replace the icon as well ?
I wound expect to pass their own icon as children here instead of having to use overrides={{icon: fn}}. IMO we should have only 1 way to changing the children via the overrides or via passing childrne but not combination of both.
If people want to add label they can do it via overrides={icon: ()<> previous</>}} or something like this, in that way they might want to go only with labels or change positions of the icon and label.

mutebg · 2023-03-20T09:19:51Z

src/pagination/__tests__/pagination.stories.tsx

+                itemButton: () => null,
+                itemDescription: ({
+                  pageNumber,
+                  lastPage,
+                  ...textBlockStyles
+                }) => (
+                  <TextBlock {...textBlockStyles}>
+                    Page {pageNumber} of {lastPage}
+                  </TextBlock>
+                ),


I think using PaginationItems here does makes whole composability useless, isn't it?
To me the idea of composability is to be able to put / replace any content with you own

<Pagianation> <PaginationFirstItem /> <PaginationPrevItem /> <PaginationListItem> Page 1 of 10 </PaginationListItem> //or <li>Page 1 of 10</li> // or <li><Form><Select /></Form></li> <PaginationNextItem /> <PaginationLastItem /> </Pagination>

If we add render methonds ( which overrides are ) and allow people to render what-ever they want via our componetns, do we really need composable Pagination?

feat(PPDSC-2839): pagination support for custom components

91d447a

github-actions bot added the feature This change contains a new feature label Mar 14, 2023

JohnTParsons added the Requires design review label Mar 14, 2023

JohnTParsons added 9 commits March 14, 2023 10:48

feat(PPDSC-2839): fix stories lint errors

e940b8a

feat(PPDSC-2839): fix stories aria errors

43a16f6

feat(PPDSC-2839): fix last label position

34e948c

feat(PPDSC-2839): fix font weight and defaults retrieval

eee0d8d

feat(PPDSC-2839): fix input focus spacing and border colour

e7fcb23

feat(PPDSC-2839): lint issues

d93de71

feat(PPDSC-2839): use both utilityButton and utilityBody fonts

1b34d94

feat(PPDSC-2839): update snapshots

f63a65a

feat(PPDSC-2839): fix ref type

1c9bfed

JohnTParsons marked this pull request as ready for review March 16, 2023 15:50

JohnTParsons requested a review from a team as a code owner March 16, 2023 15:50

feat(PPDSC-2839): use paginationlistitem in navigation components

e10cc2f

JohnTParsons added ready for review Please assist in getting this reviewed and removed Requires design review labels Mar 17, 2023

mutebg reviewed Mar 17, 2023

View reviewed changes

JohnTParsons mentioned this pull request Mar 17, 2023

docs(PPDSC-2872): pagination documentation for defaults & customisations #712

Closed

8 tasks

JohnTParsons added 4 commits March 17, 2023 13:39

feat(PPDSC-2839): add pagination to exported util names

4eb628b

feat(PPDSC-2839): useReactKeys and ScreenReaderOnly fixes

2ea49ab

feat(PPDSC-2839): useTheme import fixes

3ef8b27

feat(PPDSC-2839): replace stack with gridlayout

1709896

mstuartf previously approved these changes Mar 17, 2023

View reviewed changes

feat(PPDSC-2839): fixed missing li element in test fixture

0d365fe

JohnTParsons dismissed mstuartf’s stale review via 0d365fe March 17, 2023 17:07

feat(PPDSC-2839): paginationItemInteractive removed from defaults

d1b241a

mutebg reviewed Mar 20, 2023

View reviewed changes

feat(PPDSC-2839): screenreadonly invalid props

b5d49eb

mutebg approved these changes Mar 20, 2023

View reviewed changes

mstuartf approved these changes Mar 20, 2023

View reviewed changes

Merge branch 'main' into feat/PPDSC-2839-custom-pagination

60efc80

JohnTParsons merged commit f101abc into main Mar 20, 2023

JohnTParsons deleted the feat/PPDSC-2839-custom-pagination branch March 20, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(PPDSC-2839): pagination support for custom components #707

feat(PPDSC-2839): pagination support for custom components #707

JohnTParsons commented Mar 14, 2023 •

edited

Loading

pp-serviceaccount commented Mar 14, 2023

mutebg Mar 17, 2023

JohnTParsons Mar 17, 2023

mutebg Mar 17, 2023

JohnTParsons Mar 17, 2023

mutebg Mar 17, 2023

JohnTParsons Mar 17, 2023

mutebg Mar 17, 2023

JohnTParsons Mar 17, 2023

mutebg Mar 17, 2023

JohnTParsons Mar 17, 2023 •

edited

Loading

mutebg Mar 20, 2023

mutebg Mar 20, 2023

		itemButton?: Override<ButtonOrButtonLinkProps & PaginationItemProps>;
		itemDescription?: Override<TextBlockProps & PaginationItemDescriptionProps>;

feat(PPDSC-2839): pagination support for custom components #707

feat(PPDSC-2839): pagination support for custom components #707

Conversation

JohnTParsons commented Mar 14, 2023 • edited Loading

pp-serviceaccount commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnTParsons Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnTParsons commented Mar 14, 2023 •

edited

Loading

JohnTParsons Mar 17, 2023 •

edited

Loading