refactor: Upgrade react and react-dom to v16.8 and refactor Avatar #1171
Conversation
|
Verified that @j3tan has signed the CLA. Thanks for the pull request! |
| @@ -73,12 +73,7 @@ class Assignees extends React.Component<Props> { | |||
| /> | |||
| } | |||
| > | |||
| <AssigneeStatus | |||
There was a problem hiding this comment.
for whatever reason, the data-testid in the wrapper.find was returning both this top level function and the nested div causing 2x the number of objects returned. moving the data-testid into the child component fixes that without impacting usage
| @@ -164,10 +164,6 @@ describe('elements/content-sidebar/ContentSidebar', () => { | |||
| }); | |||
|
|
|||
| test('should set the state with the file and view and then call fetchMetadata', () => { | |||
| wrapper = getWrapper(); | |||
There was a problem hiding this comment.
unnecessary due to beforeEach, also was causing a random failure
There was a problem hiding this comment.
I think the use of hooks in Avatar is a reasonable basic example of the feature. The additional arbitrary complexity and boilerplate introduced into the tests isn't very palatable, though.
Finally, chore commits don't mix very well with breaking changes, since that scope doesn't usually trigger new releases. Consider using feat, refactor, etc., instead.
package.json
Outdated
| @@ -175,7 +175,7 @@ | |||
| "deepmerge": "^2.1.1", | |||
| "draft-js": "^0.10.1", | |||
| "enzyme": "3.8.0", | |||
| "enzyme-adapter-react-16": "^1.10.0", | |||
| "enzyme-adapter-react-16": "1.12.1", | |||
There was a problem hiding this comment.
Does this need to be pinned to this specific version?
There was a problem hiding this comment.
not fixed, can change this to ^
src/components/avatar/Avatar.js
Outdated
| avatar = <UnknownUserAvatar className="avatar-icon" />; | ||
| // Reset hasImageErrored state when avatarUrl changes | ||
| useEffect(() => { | ||
| if (hasImageErrored) { |
There was a problem hiding this comment.
Is this check necessary or should we just reset setHasImageErrored to false whenever the url changes?
There was a problem hiding this comment.
good point. since this only fires when url changes, there should be no downside to just calling this always
yeah, actually just copied the previous react upgrade PR message, but I can update to |
j3tan
changed the title
j3tan
changed the title
BREAKING CHANGE: peerDep React requirement have been bumped higher
| @@ -175,7 +175,7 @@ | |||
| "deepmerge": "^2.1.1", | |||
| "draft-js": "^0.10.1", | |||
| "enzyme": "3.8.0", | |||
| "enzyme-adapter-react-16": "^1.10.0", | |||
| "enzyme-adapter-react-16": "^1.12.1", | |||
| "enzyme-to-json": "^3.3.4", | |||
| "eslint": "^4.19.1", | |||
There was a problem hiding this comment.
Has FTL discussed adding the lint plugin for hooks?
https://www.npmjs.com/package/eslint-plugin-react-hooks
https://reactjs.org/docs/hooks-overview.html#rules-of-hooks
There was a problem hiding this comment.
yes, we will be adding this plugin. I'll discuss it at the next FTL meeting but it'll be an update to @box/frontend
BREAKING CHANGE: peerDep React requirement have been bumped higher