refactor: changed Output into a functional component w/ hooks#686
refactor: changed Output into a functional component w/ hooks#686lpizzinidev wants to merge 7 commits intouclaacm:masterfrom lpizzinidev:master
Conversation
timothycpoon
left a comment
timothycpoon
left a comment
There was a problem hiding this comment.
Mostly good, there are just a few issues, mostly with the memoization
| const [firstLoad, setFirstLoad] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| setFirstLoad(false); |
There was a problem hiding this comment.
I don't think the shouldComponentUpdate function was refactored correctly. You should pass in a second argument to react memo. Additionally, the useEffect dependency array should correspond to line 43-45 in the old file.
| } | ||
|
|
||
| if (this.props.mostRecentProgram !== nextProps.mostRecentProgram) { | ||
| this.firstLoad = true; |
There was a problem hiding this comment.
This functionality is not preserved
There was a problem hiding this comment.
setFirstLoad should be called when mostRecentProgram changes
src/components/Output/Output.js
Outdated
| ); | ||
|
|
||
| const toggleConsole = () => { | ||
| setShowConsole((prevShowConsole) => !prevShowConsole); |
There was a problem hiding this comment.
this can just be setShowConsole(!showConsole)
| ); | ||
|
|
||
| const runCode = () => { | ||
| setRun((prevRun) => prevRun + 1); |
There was a problem hiding this comment.
this should just be the run state, instead of this function
|
@lumisphere902 One note: I didn't change my |
timothycpoon
left a comment
timothycpoon
left a comment
There was a problem hiding this comment.
Couple of elaborations; also there are a few lint changes needed
| } | ||
|
|
||
| if (this.props.mostRecentProgram !== nextProps.mostRecentProgram) { | ||
| this.firstLoad = true; |
There was a problem hiding this comment.
setFirstLoad should be called when mostRecentProgram changes
| showConsole: true, | ||
| }; | ||
| this.firstLoad = true; | ||
| const compareProps = (prevProps, nextProps) => { |
There was a problem hiding this comment.
true should be returned when the component should not update; you reversed it
|
@lpizzinidev Would you like to resolve conflicts and we can merge in? Thanks! |
3 participants
Closes #670