Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(ZNTA-2610): upgrade react to 16.4.0 #882

Merged
merged 15 commits into from
Jul 2, 2018
Merged

Conversation

efloden
Copy link
Member

@efloden efloden commented Jun 7, 2018

JIRA issue URL: https://zanata.atlassian.net/browse/ZNTA-2610

Upgrade React to 16.4.0
Upgrade all affected packages.
Resolve new Jest warnings.

Checklist

  • JIRA link
  • Check target branch
  • Make sure all commit statuses are green or otherwise documented reasons to ignore
  • QA needs to evaluate against the JIRA ticket
  • Changed files and commits make sense for this PR

See Zanata Development Guidelines more for information.


This template can be updated in .github/PULL_REQUEST_TEMPLATE.md

@efloden efloden self-assigned this Jun 7, 2018
@zubot
Copy link
Member

zubot commented Jun 7, 2018

This change is Reviewable

@codecov
Copy link

codecov bot commented Jun 8, 2018

Codecov Report

Merging #882 into master will decrease coverage by 0.02%.
The diff coverage is 27.58%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #882      +/-   ##
============================================
- Coverage     36.44%   36.41%   -0.03%     
  Complexity     6002     6002              
============================================
  Files          1626     1627       +1     
  Lines         64193    64233      +40     
  Branches       7470     7474       +4     
============================================
- Hits          23395    23393       -2     
- Misses        38757    38800      +43     
+ Partials       2041     2040       -1
Impacted Files Coverage Δ Complexity Δ
...app/editor/components/TransUnitTranslationPanel.js 34.44% <ø> (-0.39%) 0 <0> (ø)
...c/app/editor/components/EditorSearchInput/index.js 85% <100%> (ø) 0 <0> (ø) ⬇️
...zanata-frontend/src/app/components/Modal/index.jsx 78.94% <100%> (+5.26%) 0 <0> (ø) ⬇️
...ntend/src/app/components/Notification/component.js 21.73% <21.73%> (ø) 0 <0> (?)
...frontend/src/app/containers/Glossary/EntryModal.js 12.5% <25%> (-0.66%) 0 <0> (ø)
...ntend/src/app/containers/ProjectVersion/MTMerge.js 29.16% <0%> (-41.67%) 0% <0%> (ø)
...p/editor/components/GlossaryTermModal/component.js 88.88% <0%> (-11.12%) 0% <0%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14107a8...6dbd005. Read the comment docs.

@alex-sl-eng
Copy link
Member

:lgtm:


Reviewed 7 of 10 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@djansen-redhat
Copy link
Contributor

This seems to break the glossary entry details button

react-dom.production.min.js:211 Uncaught TypeError: Cannot read property 'contains' of undefined
at contains.js:17
at t.B.focus (Modal.js:557)
at t.B.onShow (Modal.js:486)
at t.value (Modal.js:243)
at pr (react-dom.production.min.js:219)
at fr (react-dom.production.min.js:211)
at sr (react-dom.production.min.js:210)
at ur (react-dom.production.min.js:210)
at interactiveUpdates (react-dom.production.min.js:225)
at Ke (react-dom.production.min.js:89)

@djansen-redhat
Copy link
Contributor

I'll have to check, but I think this may also break pagination on the explore page (page not incremented)

@djansen-redhat
Copy link
Contributor

Breaks language page delete function

middleware.js:176 DELETE http://localhost:8180/rest/locales/locale/ar 405 (Method Not Allowed)
(anonymous) @ middleware.js:176
s @ runtime.js:72
(anonymous) @ runtime.js:334
e.(anonymous function) @ runtime.js:105
s @ runtime.js:72
t @ runtime.js:146
(anonymous) @ runtime.js:154
Promise.then (async)
t @ runtime.js:153
(anonymous) @ runtime.js:191
t @ $.export.js:30
r @ runtime.js:190
n @ runtime.js:211
e.(anonymous function) @ runtime.js:105
Y//3.P.async @ runtime.js:228
(anonymous) @ middleware.js:44
(anonymous) @ index.js:9
dispatch @ applyMiddleware.js:45
(anonymous) @ languages-actions.js:282
(anonymous) @ index.js:9
handleDelete @ index.js:268
n.resetSearchText @ index.js:73
onClick @ DeleteEntry.js:36
n.handleClick @ button.js:112
o @ react-dom.production.min.js:15
invokeGuardedCallback @ react-dom.production.min.js:16
invokeGuardedCallbackAndCatchFirstError @ react-dom.production.min.js:16
c @ react-dom.production.min.js:20
d @ react-dom.production.min.js:22
v @ react-dom.production.min.js:22
p @ react-dom.production.min.js:21
y @ react-dom.production.min.js:24
b @ react-dom.production.min.js:24
ze @ react-dom.production.min.js:88
mr @ react-dom.production.min.js:221
X @ react-dom.production.min.js:44
We @ react-dom.production.min.js:89
interactiveUpdates @ react-dom.production.min.js:225
Ke @ react-dom.production.min.js:89
react-dom.production.min.js:187 TypeError: Cannot read property 'contains' of undefined
at contains.js:17
at t.B.focus (Modal.js:557)
at t.B.onShow (Modal.js:486)
at t.value (Modal.js:230)
at pr (react-dom.production.min.js:218)
at fr (react-dom.production.min.js:211)
at sr (react-dom.production.min.js:210)
at ur (react-dom.production.min.js:210)
at or (react-dom.production.min.js:208)
at $n (react-dom.production.min.js:206)

@efloden
Copy link
Member Author

efloden commented Jun 12, 2018

@djansen-redhat Looks like that error is caused by the react-bootstrap modal:
react-bootstrap/react-bootstrap#2812
Just as well we are replacing them with ant components

@djansen-redhat
Copy link
Contributor

Verified

@efloden efloden merged commit bf5935d into master Jul 2, 2018
@efloden efloden deleted the react-upgrade-16/ZNTA-2610 branch July 2, 2018 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants