[BD-46] refactor Pagination component by viktorrusakov · Pull Request #1911 · openedx/paragon

viktorrusakov · 2023-01-13T07:13:17Z

Description

Refactors Pagination component to fix its style and API issues (and to improve it overall to ease maintenance).

Note: should be merged after #1190 and edx/brand-edx.org#49, as they would fix ongoing issues with styles.

Deploy Preview

https://deploy-preview-1911--paragon-openedx.netlify.app/components/pagination/

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
Does your change adhere to the documented style conventions?
Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
Were your changes tested in the example app?
Is there adequate test coverage for your changes?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

Verify your changes were released to NPM at the expected version.
If you'd like, share your contribution in #show-and-tell.
🎉 🙌 Celebrate! Thanks for your contribution.

openedx-webhooks · 2023-01-13T07:13:22Z

Thanks for the pull request, @viktorrusakov!

When this pull request is ready, tag your edX technical lead.

netlify · 2023-01-13T07:13:28Z

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`ba7deae`
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/653b55595270640008144fb7
😎 Deploy Preview	https://deploy-preview-1911--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2023-01-17T16:36:02Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (47e4379) 92.82% compared to head (ba7deae) 92.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1911      +/-   ##
==========================================
+ Coverage   92.82%   92.86%   +0.03%     
==========================================
  Files         235      246      +11     
  Lines        4237     4245       +8     
  Branches     1029     1018      -11     
==========================================
+ Hits         3933     3942       +9     
+ Misses        300      299       -1     
  Partials        4        4

Files	Coverage Δ
src/DataTable/TablePagination.jsx	`100.00% <100.00%> (ø)`
src/DataTable/TablePaginationMinimal.jsx	`85.71% <ø> (ø)`
src/Pagination/DefaultPagination.jsx	`100.00% <100.00%> (ø)`
src/Pagination/MinimalPagination.jsx	`100.00% <100.00%> (ø)`
src/Pagination/ReducedPagination.jsx	`100.00% <100.00%> (ø)`
src/Pagination/constants.js	`100.00% <100.00%> (ø)`
src/Pagination/getPaginationRange.js	`84.21% <100.00%> (-9.91%)`	⬇️
src/Pagination/index.jsx	`100.00% <100.00%> (+3.54%)`	⬆️
src/Pagination/subcomponents/Ellipsis.jsx	`100.00% <100.00%> (ø)`
src/Pagination/subcomponents/NextPageButton.jsx	`100.00% <100.00%> (ø)`
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

monteri · 2023-10-11T12:28:23Z

 import Pagination from '../Pagination';
+import { ArrowBackIos, ArrowForwardIos } from '../../icons';

 function TablePaginationMinimal() {


Can we add propTypes for this component?

This component does not receive any props, so I don't think propTypes are needed here

adamstankiewicz

Overall, this looks great! Left a few comments and clarifying questions. Also, sanity check... will we likely want to release these changes as new major version release (i.e., breaking change)?

I'm seeing at least the following possible breaking changes for consumers:

Removed SCSS variables for Pagination that theme authors may have been relying on.
icons prop now expects elements (e.g., icons imported from @edx/paragon/icons rather than nodes).

adamstankiewicz · 2023-10-13T12:39:50Z

+    leftIcon: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),
+    rightIcon: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),


[clarification] Is this intended to be a breaking change from what's supported today? In the current state, consumers can technically pass any node rather than just an element.

That said, it only looks like this impacts 1 usage of Pagination though as a breaking change according to usage insights; I think it's probably fine to go this route. That 1 usage should be using the buttonLabels instead of passing a node with <Icon> and <span className="sr-only">...</span> elements. Everything in the sr-only span really should probably be in the buttonLabels prop.

Good catch! That was a while ago, took me some time to remember why I did this haha, I was definitely trying to avoid making breaking changes, but here it's almost unavoidable I think.

The reason is that in original implementation these icon props worked only for the default variant of Pagination, i.e. they were ignored for all other variants and you could not override icons for secondary variant for example. I felt like this definitely should be fixed during this refactoring and I don't really see any other way, because we use IconButtons for other variants and they expect our icon instances, not any node.

We could probably add some magic with our withDeprecatedProps HOC to support both behaviours, but I think that would be pretty ugly... not sure if it's worth doing that, I feel like this breaking change is pretty minor and we can go with it to keep this component clean.

adamstankiewicz · 2023-10-13T12:42:05Z

+    previous: 'Previous',
+    next: 'Next',
+    page: 'Page',
+    currentPage: 'Current Page',
+    pageOfCount: 'of',


[curious] Is this a place where we should add i18n support to Pagination (could be a follow-up issue).

Yeah, it crossed my mind, but I think in this case it's harder than it seems because we use these strings in a way that does not support translation I think, e.g. the screen reader text for the component is generated as follows:

const getScreenReaderText = () => `${buttonLabels.page} ${currentPage}, ${buttonLabels.currentPage}, ${buttonLabels.pageOfCount} ${pageCount}`;

(this would generate following text Page 3, Current Page, of 20)

I'm pretty sure that for it to be translated correctly, the whole string has to be marked for translation, not just parts of it (in this case it would be page, currentPage and pageOfCount strings)

So yeah, I'm not sure that just marking these props for i18n would be useful

I think we might need to make another breaking change here to correctly support i18n, i.e. make consumers pass the whole text instead of parts of it, for example add screenReaderText prop and allow adding placeholders there, e.g. default value would be Page {currentPage}, Current Page, of {totalPagesCount} where we have predefined placeholders that will be replaced by the component with correct values. What do you think?

adamstankiewicz · 2023-10-13T12:43:33Z

+export const PAGINATION_BUTTON_LABEL_NEXT = 'Next';
+export const PAGINATION_BUTTON_LABEL_PAGE = 'Page';
+export const PAGINATION_BUTTON_LABEL_CURRENT_PAGE = 'Current Page';
+export const PAGINATION_BUTTON_LABEL_PAGE_OF_COUNT = 'of';
+export const PAGINATION_BUTTON_ICON_BUTTON_NEXT_ALT = 'Go to next page';
+export const PAGINATION_BUTTON_ICON_BUTTON_PREV_ALT = 'Go to previous page';


Also asked in another comment related to Pagination's default props, but wondering if these default English strings should be marked for i18n translation?

adamstankiewicz · 2023-10-13T12:44:18Z

+import { greaterThan } from '../utils/propTypes';
+import { ChevronLeft, ChevronRight } from '../../icons';
+
+function Pagination(props) {


Nice cleanup of the root Pagination component and abstracting out all its variants to subcomponents. Much cleaner and simpler to reason about!

adamstankiewicz · 2023-10-13T12:46:37Z

@@ -0,0 +1,7 @@
+export { default as Ellipsis } from './Ellipsis';


tiniest of nits: this could be a vanilla .js file as it doesn't render JSX

adamstankiewicz · 2023-10-13T12:48:22Z

  PAGINATION_BUTTON_LABEL_NEXT,
  PAGINATION_BUTTON_LABEL_PAGE,
-} from './Pagination';
+} from './Pagination/constants';


nit: i wonder if it's worth sticking to convention in this file of exporting constants from the top-level component file? Could consider adding the following to ./src/Pagination/index.jsx:

export * from './constants';

Yeah, makes sense, updated, thanks!

peterkulko · 2023-11-21T17:40:57Z

We plan to release all breaking changes at the same time, a new PR is open here.

openedx-webhooks · 2023-11-21T17:41:05Z

@viktorrusakov Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Jan 13, 2023

viktorrusakov marked this pull request as draft January 13, 2023 07:13

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch 3 times, most recently from 4aa622d to 0fbded8 Compare January 17, 2023 07:10

viktorrusakov marked this pull request as ready for review January 17, 2023 07:55

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch from 0fbded8 to ea7b140 Compare January 17, 2023 16:23

viktorrusakov mentioned this pull request Jan 20, 2023

Pagination component includes .btn-primary on all its buttons, when only the active page actually has .btn-primary styling #1787

Closed

2 tasks

adamstankiewicz mentioned this pull request Jan 27, 2023

[BD-46] DRAFT: fixed pagination buttons #1849

Closed

10 tasks

viktorrusakov linked an issue Apr 21, 2023 that may be closed by this pull request

Pagination component includes .btn-primary on all its buttons, when only the active page actually has .btn-primary styling #1787

Closed

2 tasks

monteri force-pushed the rusakov/attempt-to-refactor-pagination branch from ea7b140 to d90f3ce Compare July 27, 2023 13:19

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch from d90f3ce to 0895385 Compare October 10, 2023 08:35

viktorrusakov requested review from monteri and peterkulko October 10, 2023 08:43

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch from 0895385 to 43ba12f Compare October 10, 2023 08:46

peterkulko reviewed Oct 10, 2023

View reviewed changes

Comment thread src/Pagination/Pagination.test.jsx Outdated

Comment thread src/Pagination/PaginationContext.jsx Outdated

Comment thread src/Pagination/Pagination.test.jsx Outdated

Comment thread src/Pagination/constants.js

peterkulko approved these changes Oct 11, 2023

View reviewed changes

monteri reviewed Oct 11, 2023

View reviewed changes

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch from efc00db to 1ec4fa7 Compare October 13, 2023 09:22

adamstankiewicz reviewed Oct 13, 2023

View reviewed changes

adamstankiewicz mentioned this pull request Oct 23, 2023

feat: display search result cards in catalog tab openedx/frontend-app-admin-portal#1059

Merged

3 tasks

viktorrusakov and others added 6 commits October 27, 2023 09:14

feat: refactor Pagination

7eee4c2

test: refactor Pagination tests

e7d7f98

refactor: clean up after rebase

1218110

refactor: small SCSS and code style refactoring

a8e5748

test: improve code quality in Pagination tests

6247545

fix: update tests

fb4efe5

chore: update imports and prop types

ba7deae

viktorrusakov force-pushed the rusakov/attempt-to-refactor-pagination branch from b0a4053 to ba7deae Compare October 27, 2023 06:14

peterkulko mentioned this pull request Nov 21, 2023

[BD-46] refactor: refactoring Pagination component #2837

Merged

10 tasks

peterkulko closed this Nov 21, 2023

		leftIcon: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),
		rightIcon: PropTypes.oneOfType([PropTypes.element, PropTypes.func]),

		@@ -0,0 +1,7 @@
		export { default as Ellipsis } from './Ellipsis';

Uh oh!

Conversation

viktorrusakov commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

Uh oh!

openedx-webhooks commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for paragon-openedx ready!

Uh oh!

codecov Bot commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viktorrusakov Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamstankiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Nov 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viktorrusakov commented Jan 13, 2023 •

edited

Loading

openedx-webhooks commented Jan 13, 2023 •

edited

Loading

netlify Bot commented Jan 13, 2023 •

edited

Loading

codecov Bot commented Jan 17, 2023 •

edited

Loading

viktorrusakov Oct 13, 2023 •

edited

Loading

peterkulko commented Nov 21, 2023 •

edited

Loading