Skip to content
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

refactor: changed Output into a functional component w/ hooks #686

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

refactor: changed Output into a functional component w/ hooks #686

wants to merge 7 commits into from

Conversation

lpizzinidev
Copy link

Closes #670

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, there are just a few issues, mostly with the memoization

const [firstLoad, setFirstLoad] = useState(true);

useEffect(() => {
setFirstLoad(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality is not preserved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setFirstLoad should be called when mostRecentProgram changes

);

const toggleConsole = () => {
setShowConsole((prevShowConsole) => !prevShowConsole);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can just be setShowConsole(!showConsole)

);

const runCode = () => {
setRun((prevRun) => prevRun + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should just be the run state, instead of this function

@lpizzinidev
Copy link
Author

@lumisphere902
I've updated the component.
Let me know if there are other changes to be made.

One note: I didn't change my runCode function implementation because it handles the run state like before.
So I think that my refactoring is fine in that case.
Let me know if I'm missing something.

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of elaborations; also there are a few lint changes needed

}

if (this.props.mostRecentProgram !== nextProps.mostRecentProgram) {
this.firstLoad = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setFirstLoad should be called when mostRecentProgram changes

showConsole: true,
};
this.firstLoad = true;
const compareProps = (prevProps, nextProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true should be returned when the component should not update; you reversed it

@reginawang3495
Copy link
Contributor

@lpizzinidev Would you like to resolve conflicts and we can merge in? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Output.js into a functional component w/ hooks
3 participants