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

Please carefully look up to this here as setIsActive is not defined yet but called inside code at line 163 #6657

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KishorKumarParoi
Copy link

Please carefully look up to this here setIsActive is not defined yet at line 163, isActive is only passing by props from Accordion, I know that what I did it's not best solution,I added this lines for proper understanding and updated setIsActive to setCheckActive to remove errors, Please take necessary steps so that learners like myself don't get confused by ReferenceError of variables which isn't declared yet and got curious about why it was like that

…at line 163, isActive is only passing by props from Accordion, I know that what I did it's not best solution,I added this lines for proper understanding and updated setIsActive to setCheckActive to remove errors, Please take necessary steps so that learners like myself don't get confused by ReferenceError of variables which isn't declared yet and got curious about why it was like that
Copy link

vercel bot commented Feb 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
react-dev ✅ Ready (Inspect) Visit Preview Feb 25, 2024 10:21am

Copy link

github-actions bot commented Feb 22, 2024

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@pwbriggs
Copy link

Hey there @KishorKumarParoi! 👋

The error in this code is intentional. It's showing the second step of a 3-step refactoring to lift state up.

That said, I agree that this may be confusing. Perhaps it would be a good idea to add a comment next to the confusing code? Something like this:

function Panel({ title, children, isActive }) {
  return (
    <section className="panel">
      <h3>{title}</h3>
      {isActive ? (
        <p>{children}</p>
      ) : (
        <button onClick={() => setIsActive(true)}>
        // 👆 This onClick handler doesn't work yet, because we removed setIsActive in step 1
          Show
        </button>
      )}
    </section>
  );
}

What do you think?

@KishorKumarParoi
Copy link
Author

Hey there @KishorKumarParoi! 👋

The error in this code is intentional. It's showing the second step of a 3-step refactoring to lift state up.

That said, I agree that this may be confusing. Perhaps it would be a good idea to add a comment next to the confusing code? Something like this:

function Panel({ title, children, isActive }) {
  return (
    <section className="panel">
      <h3>{title}</h3>
      {isActive ? (
        <p>{children}</p>
      ) : (
        <button onClick={() => setIsActive(true)}>
        // 👆 This onClick handler doesn't work yet, because we removed setIsActive in step 1
          Show
        </button>
      )}
    </section>
  );
}

What do you think?

Yes I am agreed with you

@pwbriggs
Copy link

@KishorKumarParoi how about you update the PR with these changes then?

@KishorKumarParoi
Copy link
Author

@KishorKumarParoi how about you update the PR with these changes then?

Updated Thank you

@Mavludin
Copy link

Hope this one is going to be merged soon

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

Successfully merging this pull request may close these issues.

4 participants