Skip to content

Conversation

@ddanielsantos
Copy link

@ddanielsantos ddanielsantos commented Aug 17, 2024

Most react code written today uses functional components and hooks, with this PR, the main class component (Instagram) is converted to its equivalent functional component.

It does not lead to changes in behavior or improvements in performance, but it brings the code closer to the present day and will facilitate future maintenance by other people.

renderClose and renderWebview are still inner functions of the main component

Copy link

@noghartt noghartt left a comment

Choose a reason for hiding this comment

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

let some review comments here

if (results.access_token) {
// Keeping this to keep it backwards compatible, but also returning raw results to account for future changes.
this.props.onLoginSuccess(results.access_token, results);
onLoginSuccess(results.access_token, results);

Choose a reason for hiding this comment

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

should we assume that onLoginSuccess is required every time?

Choose a reason for hiding this comment

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

add a conditional to validate that onLoginSuccess exists

Suggested change
onLoginSuccess(results.access_token, results);
if (onLoginSuccess) {
onLoginSuccess(results.access_token, results);
}

if (responseType === 'code' && !appSecret) {
if (code) {
this.props.onLoginSuccess(code, results);
onLoginSuccess(code, results);

Choose a reason for hiding this comment

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

same here

onLoginSuccess(code, results);
} else {
this.props.onLoginFailure(results);
onLoginFailure(results);

Choose a reason for hiding this comment

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

same here


if (res) {
this.props.onLoginSuccess(res.data, results);
onLoginSuccess(res.data, results);

Choose a reason for hiding this comment

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

same here

onLoginSuccess(res.data, results);
} else {
this.props.onLoginFailure(results);
onLoginFailure(results);

Choose a reason for hiding this comment

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

same here

this.hide();
this.props.onLoginFailure(json);
hide();
onLoginFailure(json);

Choose a reason for hiding this comment

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

same here

webViewState.url === 'https://www.instagram.com/'
) {
this.setState({ key: key + 1 });
setKey((k) => k + 1);

Choose a reason for hiding this comment

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

why exactly do you need to increase the key counter on that? just to force a rerender?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't understand too

Should we do this change in this PR, since its only goal is to move from class to FC?

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.

2 participants