Pagination Front End - Part 1 #9937

RetiredBlob · 2022-03-10T07:13:36Z

Part of #9943

The 240 breakpoint design will be done in a separate PR, since this PR is already quite big

I have assigned myself to this PR and the corresponding issues
I have added the cross-team label to this PR if it requires visibility across World Service teams
I have assigned this PR to the Simorgh project
(BBC contributors only) This PR follows the repository use guidelines

Testing:

Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)
This PR requires manual testing

RetiredBlob · 2022-03-15T10:04:43Z

src/app/pages/TopicPage/Pagination/index.jsx

+  background: #696969;
+`;
+const UnavailableBlock = styled(Block)`
+  color: #bababa;


src/app/pages/TopicPage/TopicPage.jsx

rebeccamcginn · 2022-03-23T16:45:30Z

The ellipsis between 1 and 2 feels like a bug. I know it's most likely in the designs but... no number is missing!

JonBeeb · 2022-03-24T18:00:39Z

Er, I would leave it as it is for now and keep the same logic is Webcore?

jroebu14 · 2022-03-25T08:34:25Z

src/app/pages/TopicPage/Pagination/index.jsx

+    </EllipsisBlock>
+  ),
+};
+/* eslint-enable react/prop-types */


Looks like we have prop types. Can we remove this?

This is an eslint-enable - it corresponds with the eslint-disable on line 97

andrewscfc

Not quite finished reviewing this but just dropping what I've found so far, hope it helps!

src/app/lib/utilities/findNClosestIndicies/index.js

andrewscfc · 2022-03-25T09:44:40Z

src/app/pages/TopicPage/Pagination/buildBlocks.js

+    activePage,
+    pageCount,
+    activePageOnEdge: activePage === 1 || activePage === pageCount,
+  };


When I first looked at this file I was pretty unclear where state is initialised (here I eventually found). I'm unsure of the need for a global state variable in this case 🤔 Can we chain the functions so state is not mutated globally?

On this line for example we could reduce rather than map arrive at the final state value, see below for a sketch of what I might suggest

I feel it reads better and elimates the risk of errors being introduced later when working with global state.

andrewscfc · 2022-03-25T10:01:45Z

src/app/pages/TopicPage/Pagination/buildBlocks.js

+  if (pageCount <= 1) return null;
+  state = {
+    activePage,
+    pageCount,
+    activePageOnEdge: activePage === 1 || activePage === pageCount,
+  };
+  state.result = Array(pageCount)
+    .fill()
+    .map((_, i) => createPage(i));
+
+  setRequiredVisibility();
+  setDynamicVisibility();
+  pruneInvisible();
+  insertEllipsis();
+  insertArrows();
+  addKeys();
+
+  return state.result;


Suggested change

if (pageCount <= 1) return null;

state = {

activePage,

pageCount,

activePageOnEdge: activePage === 1 || activePage === pageCount,

};

state.result = Array(pageCount)

.fill()

.map((_, i) => createPage(i));

setRequiredVisibility();

setDynamicVisibility();

pruneInvisible();

insertEllipsis();

insertArrows();

addKeys();

return state.result;

if (pageCount <= 1) return null;

const intialState = {

activePage,

pageCount,

activePageOnEdge: activePage === 1 || activePage === pageCount,

};

//state is no longer global

const state = Array(pageCount)

.fill()

.reduce((currentState, i) => createPage(i, currentState), intialState);

return pipe(setRequiredVisibility, setDynamicVisibility, pruneInvisible, insertEllipsis, insertArrows, addKeys)(state).result;

btw, fully aware this code won't 'just work', just illustrating the general pattern

andrewscfc · 2022-03-25T10:48:15Z

src/app/pages/TopicPage/Pagination/buildBlocks.js

+  state.result.forEach((result, i) => {
+    // eslint-disable-next-line no-param-reassign
+    result.key = i;
+  });


Suggested change

state.result.forEach((result, i) => {

// eslint-disable-next-line no-param-reassign

result.key = i;

});

state.result = state.result.map((result, i) => {

return { ...result, key: i };

});

Think this will remove the need for the linting comment and make this more functional

👍 this is a bigger problem since moving state to being passed into each function

really don't think it's worth the added code complexity to try to make all of these helper functions pure

they're all private - the default export is pure

andrewscfc

Think I've finished my review of this, let me know if you need any help or clarification. Happy to debate the points as well, I know some of my suggestions result in significant changes.

andrewscfc · 2022-03-25T11:49:08Z

src/app/pages/TopicPage/Pagination/index.jsx

+
+const ActiveBlock = styled(Block)`
+  color: ${C_WHITE};
+  background: #696969;


I know it's annoying but I think these colours should be added to psammead styles or we should reference existing colours? As we make chameleon changes it'll be very difficult to make global changes without doing this

I added this here RetiredBlob@50a2cde

andrewscfc · 2022-03-25T11:52:59Z

src/app/pages/TopicPage/Pagination/index.jsx

+
+const Pagination = ({ activePage, pageCount }) => {
+  const { service } = useContext(ServiceContext);
+  const clampedPageCount = clamp(1, 99, pageCount);


You could consider maintaining the max page count in the BFF to keep this decision in one place in case we change it in the future?

Also there would be no need for the clamp logic as the BFF would only return up to 40 pages and 404 if any page higher (or lower) were requested

andrewscfc · 2022-03-25T11:58:19Z

src/app/pages/TopicPage/Pagination/index.jsx

+  pageCount: 1,
+};
+
+export default Pagination;


This component needs some tests to show it can take varying page ranges and return suitable visual components. It's not about retesting buildBlocks as that has a lot of the complex view logic, but checking that the correct dom elements are rendered for different page ranges. It doesn't need to be as comprehensive as the buildBlocks test.

JonBeeb · 2022-03-28T10:47:32Z

Updated UX /a11y captured in this ticket: #9984

greenc05 · 2022-03-28T15:33:45Z

src/app/pages/TopicPage/Pagination/index.jsx

+  text-decoration: none;
+  height: 100%;
+  width: 100%;
+  &:hover {


Missing focus?

greenc05 · 2022-03-28T15:34:16Z

src/app/pages/TopicPage/Pagination/index.jsx

+  if (!blocks) return null;
+
+  return (
+    <OL role="list">


Should 'OL' be in caps?

src/app/pages/TopicPage/Pagination/index.jsx

Co-authored-by: Jonathan Roebuck <[email protected]>

RetiredBlob added 3 commits March 10, 2022 00:27

this sucks

f6b2695

Almost

6570c4b

Working

6205865

ryanmccombe mentioned this pull request Mar 11, 2022

[Topic Pagination] Front End #9943

Closed

RetiredBlob and others added 2 commits March 15, 2022 09:59

Productionise

ac9d197

Merge branch 'latest' into pagination-bad

e4f1e69

RetiredBlob changed the title ~~Pagination bad~~ Pagination Front End - Part 1 Mar 15, 2022

RetiredBlob marked this pull request as ready for review March 15, 2022 10:02

RetiredBlob commented Mar 15, 2022

View reviewed changes

ryanmccombe assigned RetiredBlob Mar 15, 2022

ryanmccombe added the ws-glasgow Work pertaining to the World Service Glasgow team label Mar 15, 2022

RetiredBlob and others added 4 commits March 15, 2022 10:36

Lint

078975d

Fix unit tests

900ea36

Lint

ee89457

Merge branch 'latest' into pagination-bad

7bdb6b2

rebeccamcginn reviewed Mar 22, 2022

View reviewed changes

src/app/pages/TopicPage/TopicPage.jsx Outdated Show resolved Hide resolved

rebeccamcginn reviewed Mar 22, 2022

View reviewed changes

src/app/pages/TopicPage/TopicPage.jsx Outdated Show resolved Hide resolved

rebeccamcginn added 2 commits March 22, 2022 15:19

Update src/app/pages/TopicPage/TopicPage.jsx

d92d225

Update src/app/pages/TopicPage/TopicPage.jsx

fcda152

jroebu14 reviewed Mar 25, 2022

View reviewed changes

jroebu14 approved these changes Mar 25, 2022

View reviewed changes

andrewscfc reviewed Mar 25, 2022

View reviewed changes

JonBeeb added this to the Pagination milestone Mar 28, 2022

greenc05 reviewed Mar 28, 2022

View reviewed changes

jroebu14 mentioned this pull request Mar 29, 2022

Add new colours BBC-archive/psammead#4632

Merged

4 tasks

RetiredBlob and others added 2 commits April 3, 2022 19:02

Merge branch 'latest' into pagination-bad

088c92c

CR feedback

679badc

jroebu14 reviewed Apr 4, 2022

View reviewed changes

src/app/pages/TopicPage/Pagination/index.jsx Outdated Show resolved Hide resolved

RetiredBlob and others added 2 commits April 4, 2022 08:49

Update src/app/pages/TopicPage/Pagination/index.jsx

45a6b8d

Co-authored-by: Jonathan Roebuck <[email protected]>

Merge branch 'latest' into pagination-bad

cbc929f

rebeccamcginn approved these changes Apr 5, 2022

View reviewed changes

ryanmccombe merged commit 10db592 into bbc:latest Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination Front End - Part 1 #9937

Pagination Front End - Part 1 #9937

RetiredBlob commented Mar 10, 2022 •

edited

Loading

RetiredBlob Mar 15, 2022

rebeccamcginn commented Mar 23, 2022

JonBeeb commented Mar 24, 2022

jroebu14 Mar 25, 2022

RetiredBlob Apr 4, 2022

andrewscfc left a comment

andrewscfc Mar 25, 2022 •

edited

Loading

andrewscfc Mar 25, 2022

andrewscfc Mar 25, 2022

andrewscfc Mar 25, 2022

RetiredBlob Apr 4, 2022

andrewscfc left a comment

andrewscfc Mar 25, 2022

jroebu14 Mar 29, 2022

andrewscfc Mar 25, 2022 •

edited

Loading

andrewscfc Mar 25, 2022

JonBeeb commented Mar 28, 2022

greenc05 Mar 28, 2022

greenc05 Mar 28, 2022

Pagination Front End - Part 1 #9937

Pagination Front End - Part 1 #9937

Conversation

RetiredBlob commented Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

rebeccamcginn commented Mar 23, 2022

JonBeeb commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewscfc left a comment

Choose a reason for hiding this comment

andrewscfc Mar 25, 2022 • edited Loading

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

andrewscfc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewscfc Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonBeeb commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RetiredBlob commented Mar 10, 2022 •

edited

Loading

andrewscfc Mar 25, 2022 •

edited

Loading

andrewscfc Mar 25, 2022 •

edited

Loading