-
Notifications
You must be signed in to change notification settings - Fork 377
Update React 16.4 (mixed versions) to React 16.8 #1541
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1541 +/- ##
=========================================
+ Coverage 83.62% 83.82% +0.2%
=========================================
Files 552 556 +4
Lines 5751 5856 +105
Branches 12 12
=========================================
+ Hits 4809 4909 +100
- Misses 940 945 +5
Partials 2 2
Continue to review full report at Codecov.
|
|
@redallen - we'd like to get this tested with two other products to ensure there are no breaking changes. Also - what is left to get the do not merge label removed? |
|
All that's left is getting gatsby doc generation working. Weird typescript-related errors are occurring. |
bd4d925 to
e7e1154
Compare
|
PatternFly-React preview: https://1541-pr-patternfly-react-patternfly.surge.sh |
dgutride
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally - I think this looks good, mostly snapshot updates. In the future, it would be good to break these into much smaller PR's to ease reviewing. For instance - could we have only updated PF4 at this time - should we make sure any react dependencies are decoupled between pf3 and pf4 to allow them to move independently?
|
PatternFly-React preview: https://1541-pr-patternfly-react-patternfly.surge.sh |
| /> | ||
| > | ||
| <Component /> | ||
| </GenerateId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pre-existing problem, but we should probably revisit these snapshot tests later. A snapshot of a shallow render of something wrapped by GenerateId or ContextConsumer isn't accomplishing anything.
...ages/patternfly-4/react-core/src/components/Dropdown/__snapshots__/DropdownItem.test.js.snap
Show resolved
Hide resolved
| </Table> | ||
| </Section> | ||
| ); | ||
| // {props.map(prop => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid committing commented-out code, right?
Also, when trying to check the effects of removing this, I went to look at the props table for Avatar and on https://1541-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/avatar I'm getting a blank page with console errors:
TypeError: Cannot read property 'name' of null
at getEnumValue (propsTable.js:68)
at propsTable.js:53
at Array.map (<anonymous>)
at PropsTable (propsTable.js:49)
at hg (react-dom.production.min.js:150)
at Tg (react-dom.production.min.js:178)
at bi (react-dom.production.min.js:232)
at ci (react-dom.production.min.js:233)
at Di (react-dom.production.min.js:249)
at Yh (react-dom.production.min.js:248)
qh @ react-dom.production.min.js:198
react-dom.production.min.js:248 Uncaught TypeError: Cannot read property 'name' of null
at getEnumValue (propsTable.js:68)
at propsTable.js:53
at Array.map (<anonymous>)
at PropsTable (propsTable.js:49)
at hg (react-dom.production.min.js:150)
at Tg (react-dom.production.min.js:178)
at bi (react-dom.production.min.js:232)
at ci (react-dom.production.min.js:233)
at Di (react-dom.production.min.js:249)
at Yh (react-dom.production.min.js:248)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest commit by changing a little bit of Gatsby logic. New preview should deploy shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New preview doesn't seem to have console errors for Avatar: https://1541-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/avatar
packages/patternfly-4/react-core/scripts/convertComponentHelpers.js
Outdated
Show resolved
Hide resolved
mturley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work @redallen, I especially love the scripts for converting components to TS! Just left a couple of minor comments.
dlabaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
mturley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Can't wait to build some stuff with Hooks.
What:
Additional issues:
Pf4 doc build is broken. Working with @jschuler to fix.Fixed