feat: update react & react-dom to v17 #758
Conversation
…m/openedx/frontend-app-learner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #758 +/- ##
==========================================
+ Coverage 84.87% 85.05% +0.18%
==========================================
Files 320 324 +4
Lines 6399 6698 +299
Branches 1552 1623 +71
==========================================
+ Hits 5431 5697 +266
- Misses 941 970 +29
- Partials 27 31 +4
☔ View full report in Codecov by Sentry. |
BilalQamar95
changed the title
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
package.json
Outdated
| "react-instantsearch-dom": "6.38.1", | ||
| "react-lines-ellipsis": "0.15.3", |
There was a problem hiding this comment.
Would recommend trying to use Paragon's Truncate component, if possible, rather than relying on a new third-party library.
There was a problem hiding this comment.
Sure that's better, I have replaced it with Paragon's Truncate
| @@ -81,7 +77,7 @@ describe('<SearchProgramCard />', () => { | |||
| const { container } = renderWithRouter(<SearchProgramCardWithAppContext {...defaultProps} />); | |||
|
|
|||
| expect(screen.getByText(PROGRAM_TITLE)).toBeInTheDocument(); | |||
| expect(screen.getByText(PROGRAM_AUTHOR_ORG.key)).toBeInTheDocument(); | |||
| expect(screen.getAllByText(PROGRAM_AUTHOR_ORG.key).length).toBeGreaterThan(0); | |||
There was a problem hiding this comment.
[curious] Can you explain the rationale for this change?
There was a problem hiding this comment.
The test was failing after switch to react-lines-ellipsis, as the text within the "LinesEllipsis" component is being broken into multiple parts and being rendered multiple times in the view. This is no longer an issue after switch to Paragon Truncate & I have reverted the change.
Explanation: react-lines-ellipsis employs multiple hidden nodes to calculate which parts of the text to show or hide. It breaks down the text into smaller units uses this to determine the ellipsis effect. This was resulting in multiple parts of the DOM containing the same text string, which is why getByText was throwing an error as it was finding multiple instances of the expected text.
| @@ -145,7 +140,7 @@ describe('<SearchProgramCard />', () => { | |||
|
|
|||
| expect(screen.getByText(PROGRAM_TITLE)).toBeInTheDocument(); | |||
| expect(screen.getByAltText(PROGRAM_AUTHOR_ORG.name)).toBeInTheDocument(); | |||
| expect(screen.getByText(PROGRAM_AUTHOR_ORG.name)).toBeInTheDocument(); | |||
| expect(screen.getAllByText(PROGRAM_AUTHOR_ORG.name).length).toBeGreaterThan(0); | |||
There was a problem hiding this comment.
[curious] Can you explain the rationale for this change?
There was a problem hiding this comment.
The test was failing after switch to react-lines-ellipsis, as the text within the "LinesEllipsis" component is being broken into multiple parts and being rendered multiple times in the view. This is no longer an issue after switch to Paragon Truncate & I have reverted the change.
Explanation: react-lines-ellipsis employs multiple hidden nodes to calculate which parts of the text to show or hide. It breaks down the text into smaller units uses this to determine the ellipsis effect. This was resulting in multiple parts of the DOM containing the same text string, which is why getByText was throwing an error as it was finding multiple instances of the expected text.
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
There was a problem hiding this comment.
LGTM! Checked out your branch and ran the app locally and it's working as expected. Just a couple of nits but nothing blocking.
[nit]: The description is showing the UI with react-truncate -- can we replace that with Paragon's Truncate component instead?
[nit]: I think we can also remove @fortawesome/react-fontawesome from the description since we're no longer using that dependency.
…ner-portal-enterprise into bilalqamar95/react-upgrade-to-v17
Ticket
Upgrade React JS to v17
Description
react&react-domto v17 along withreact-test-renderer,react-helmet&@testing-library/reactto respective compatible versionsenzyme-adapter-react-16with@wojtekmaj/enzyme-adapter-react-17@edx/frontend-platform, @edx/frontend-build, @edx/frontend-enterprisereact-truncatewithreact-lines-ellipsisTruncatedue to dependency issues with react v17With react-truncate
With Paragon's Truncate
For all changes
Only if submitting a visual change