build(ui): Upgrade to React 16 #6984
Conversation
Generated by 🚫 danger |
lynnagara
force-pushed
the
react-16
branch
3 times, most recently
from
4ca83c8 to
556e0d0
Compare
|
I don't see any differences in the percy snapshots |
|
Can we fix the requestionAnimationFrame errors? It seems it's fixed in a newer jest, but we could polyfill it until upgrade: jestjs/jest#4545 |
|
And have you tested on staging? |
|
@billyvg - Yep, just found 1 issue - https://github.com/getsentry/getsentry/pull/1662 |
|
Gonna update and redeploy to staging |
|
@billyvg is there a reason we can't upgrade jest instead? |
|
Yeah I'll throw up a jest 22 PR |
lynnagara
changed the title
lynnagara
force-pushed
the
react-16
branch
2 times, most recently
from
7807ccf to
b247615
Compare
|
|
||
| import createReactClass from 'create-react-class'; | ||
|
|
||
| import ApiMixin from '../../mixins/apiMixin'; | ||
|
|
||
| import {update as projectUpdate} from '../../actionCreators/projects'; | ||
|
|
||
| import LatestContextStore from '../../stores/latestContextStore'; | ||
|
|
||
| const BookmarkToggle = createReactClass({ |
There was a problem hiding this comment.
maybe leave a comment that this is coupled to the current project now? the implementation looks good
| getInitialState() { | ||
| return { | ||
| orgId: null, | ||
| project: null, |
There was a problem hiding this comment.
showing my ignorance of reflux, but do you need to pull the current state of the store out here in case it isn't updated after the component is mounted?
There was a problem hiding this comment.
It always "just works" since the component is mounted faster than the API calls return and trigger onLatestContextUpdate. I could add it to the initial state to be safe though?
There was a problem hiding this comment.
That works. Updated for Reflux.connect
|
|
||
| let isActive = this.state.project ? this.state.project.isBookmarked : false; | ||
|
|
||
| let projectIconClass = classNames('project-select-bookmark icon icon-star-solid', { |
There was a problem hiding this comment.
it's nicer that this handles its own class now
There was a problem hiding this comment.
awesome. snapshots and changes look good.
My main fears are
A) more context render triggering related issues
and
B) shifted behavior from bumped dependencies like react-bootstrap, which drives our modals. i notice they looked different in the snaps so i think they're also worth manually playing with.
if i was more afraid I would say some of these changes could be merged separately to break up the risk but i think it will be fine
| @@ -61,20 +61,19 @@ | |||
| "prop-types": "^15.6.0", | |||
| "query-string": "2.4.2", | |||
| "raven-js": "3.19.1", | |||
| "react": "15.6.2", | |||
| "react": "16.2.0", | |||
| "react-bootstrap": "^0.29.5", | ||
| "react-document-title": "1.0.4", | ||
| "react-dom": "15.6.2", | ||
| "react-bootstrap": "^0.32.0", |
There was a problem hiding this comment.
nice, does this fix the warnings?
There was a problem hiding this comment.
i noticed it fixed some warnings relating to modals....
| @@ -46,7 +46,7 @@ export default class ActionLink extends React.Component { | |||
| return ( | |||
| <a | |||
| className={classNames(className, {disabled})} | |||
| onClick={!disabled && onAction} | |||
| onClick={disabled ? undefined : onAction} | |||
There was a problem hiding this comment.
react doesn't like false as a click handler now?
Context changes do not always trigger a rerender
Depends on
https://github.com/getsentry/getsentry/pull/1660andhttps://github.com/getsentry/getsentry/pull/1662