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
update to Babel v7 #1048
update to Babel v7 #1048
Conversation
cb991f0
to
3d9e88a
Compare
…d correct babel config resolve
package.json
Outdated
@@ -112,7 +113,7 @@ | |||
"react-mentions": "^1.2.1", | |||
"react-redux": "^5.0.1", | |||
"react-syntax-highlighter": "^6.0.2", | |||
"react-virtualized": "^9.18.5", | |||
"react-virtualized": "^9.21.0", |
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.
Any reason this was upgraded?
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.
I was running into failures on jest with react-virtualized's scrollbarSize
not being found. I thought updating it to latest would fix it, however the problem persisted.
Looking into it, there was a version mismatch of a dependency (dom-helpers
) with react-virtualized
and momentum-ui
where momentum-ui was still using the older version of dom-helpers
.
"Sticking" react-virtualized
to 9.21.0
keeps the dom-helpers
dependency inline with the same version that momentum-ui
uses.
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.
Retried without bumping, problem isnt showing up anymore, so reverting back 9.18.5
packages/node_modules/@ciscospark/react-component-activity-item/src/index.js
Outdated
Show resolved
Hide resolved
@@ -17,6 +17,8 @@ const propTypes = { | |||
file: PropTypes.shape({ | |||
displayName: PropTypes.string, | |||
fileSize: PropTypes.number, | |||
image: PropTypes.shape(), |
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 should be object.
image: PropTypes.shape(), | |
image: PropTypes.object, |
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.
I think I originally made this a shape since, I believe, there are two properties associated with image, height
& width
, and added them into the proptype, however wasnt sure if height & width were being passed as strings or numbers
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.
Right, but a shape prop type indicates that you will provide an actual shape. shape()
doesn't tell us anything about the property, so let's just use the ambiguous .object
packages/node_modules/@ciscospark/react-component-file-staging-area/src/index.js
Show resolved
Hide resolved
packages/node_modules/@webex/react-component-activity-item/src/index.js
Outdated
Show resolved
Hide resolved
@@ -17,6 +17,8 @@ const propTypes = { | |||
file: PropTypes.shape({ | |||
displayName: PropTypes.string, | |||
fileSize: PropTypes.number, | |||
image: PropTypes.shape(), |
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.
image: PropTypes.shape(), | |
image: PropTypes.object(), |
Good first pass, nice to see all the tests passing! Comments are mostly around the react prop changes. |
9e5fc79
to
a04703f
Compare
Squashed commits: [4328e19d] chore(lint): fix all eslint error
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!
No description provided.