fix: remove support for component labels in Pagination #775
Conversation
BREAKING CHANGE: This commit removes support for passing React components as `buttonLabels` in the Pagination component. `buttonLabels` must now be strings. If you were using `FormattedMessage` to pass a label into Pagination, then you must use `intl.formatMessage` instead so that the return value is a string. This removes our dependency on `sanitize-html`, which is a package we were using to turn component-based labels back into plain strings. `sanitize-html` v2.x is compatible with PostCSS 8, a requirement for our upgrade to webpack 5. Unfortunately v2.x also drops ES5 support, which we also need. So the dependency needed to go. To upgrade your components, pass plain string labels to `buttonLabels` instead of components or JSX. There are no known places where people are doing this, so it should be a no-op for known MFEs.
| @@ -21,6 +21,24 @@ describe('<Pagination />', () => { | |||
| expect(wrapper.exists()).toEqual(true); | |||
| }); | |||
|
|
|||
| it('renders screen reader section', () => { | |||
There was a problem hiding this comment.
This wasn't tested, so I added this so I could validate my change.
| @@ -371,7 +326,7 @@ Pagination.propTypes = { | |||
| /** specifies the total number of pages in the `Pagination` component. */ | |||
| pageCount: PropTypes.number.isRequired, | |||
| /** specifies the `aria-label` for the `<nav>` element that wraps the pagination button list. */ | |||
| paginationLabel: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired, | |||
| paginationLabel: PropTypes.string.isRequired, | |||
There was a problem hiding this comment.
I noted that this label also specified it could be an element, but that it stuffs the value directly into an aria-label attribute. There's no way that could be anything other than a string, so I removed the erroneous prop type.
| @@ -3487,6 +3487,7 @@ | |||
| "version": "3.2.1", | |||
| "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-3.2.1.tgz", | |||
| "integrity": "sha512-VT0ZI6kZRdTh8YyJw3SMbYm/u+NqfsAxEpWO0Pf9sq8/e94WxxOpPKx9FR1FlyCtOVDNOQ+8ntlqFxiRc+r5qA==", | |||
| "dev": true, | |||
There was a problem hiding this comment.
All of these "dev": true are telling - sanitize-html had dependencies that were generally only dev dependencies... but we were adding them into our app. Ouch.
| @@ -13916,16 +13855,6 @@ | |||
| "integrity": "sha1-AerA/jta9xoqbAL+q7jB/vfgDqs=", | |||
| "dev": true | |||
| }, | |||
| "postcss": { | |||
There was a problem hiding this comment.
This line is the payoff of this entire thing - we need this dependency to go away! It's preventing us from upgrading to webpack 5, since the latest and greatest packages in the ecosystem all require PostCSS 8, and it gets installed as a peer dependency. Dependencies like this line were superceding our attempts at installing PostCSS 8 and breaking everything.
| @@ -49,7 +49,6 @@ | |||
| "react-responsive": "^6.1.1", | |||
| "react-table": "^7.6.1", | |||
| "react-transition-group": "^4.0.0", | |||
| "sanitize-html": "^1.27.5", | |||
There was a problem hiding this comment.
Goodnight, sweet prince.
|
🎉 This PR is included in version 16.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
BREAKING CHANGE:
This commit removes support for passing React components as
buttonLabelsorpaginationLabelin the Pagination component. The values of both attributes must now be strings. If you were usingFormattedMessageto pass a label into Pagination, then you must useintl.formatMessageinstead so that the return value is a string.This removes our dependency on
sanitize-html, which is a package we were using to turn component-based labels back into plain strings.sanitize-htmlv2.x is compatible with PostCSS 8, a requirement for our upgrade to webpack 5. Unfortunately v2.x also drops ES5 support, which we also need. So the dependency needed to go.To upgrade your components, pass plain string labels into
buttonLabelsorpaginationLabelinstead of components or JSX. There are no known places where people are doing this, so it should be a no-op for known MFEs.