-
Notifications
You must be signed in to change notification settings - Fork 6
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
OE-818: Update React, Node, Chakra, and other packages #157
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks good! Just the one comment about the peer dependencies
package.json
Outdated
"peerDependencies": { | ||
"@chakra-ui/react": "^1.8.7 || 2.x", | ||
"@emotion/react": "^11.8.2", | ||
"@emotion/styled": "^11.8.1", | ||
"react": "^16.8.0 || 17.x || 18.x", | ||
"react-dom": "^16.8.0 || 17.x || 18.x", | ||
"swr": "^2.0.3" |
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 these should stay as peer dependencies rather than production dependencies because it allows the dependency to be satisfied by an installing repo having a similar version
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 do think we can keep these, but they should be updated to require react 18 and chakra v2. Using chakra v2 in the consuming app while the web reader was on chakra v1 was causing some very strange styling issues.
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.
Oh yes true. The updates were good, just should stay peer instead of production dependencies
OE-818
Tests for single- and multi- pdf are failing due to DRB being behind the VPN