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

feat: adding lessons for else if to pyramid project #54830

Conversation

jdwilkin4
Copy link
Contributor

Summary

The first review project mentions that else if statements were taught in the pyramid. But that ins't the case, So this PR adds that lesson.

Checklist:

Closes #54825

@jdwilkin4 jdwilkin4 self-assigned this May 16, 2024
@jdwilkin4 jdwilkin4 requested a review from a team as a code owner May 16, 2024 06:31
@jdwilkin4 jdwilkin4 added the new javascript course These are for issues dealing with the new JS curriculum label May 16, 2024
@github-actions github-actions bot added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label labels May 16, 2024
@jdwilkin4 jdwilkin4 added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label May 16, 2024
Copy link
Contributor

@JoyShaheb JoyShaheb left a comment

Choose a reason for hiding this comment

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

All good Jess ! just a few spelling mistakes here and there. Good work 👍

jdwilkin4 and others added 2 commits May 17, 2024 09:17
Co-authored-by: Joy Shaheb <khondokoralam@gmail.com>
@jdwilkin4 jdwilkin4 requested a review from JoyShaheb May 17, 2024 16:44
Copy link
Contributor

@lasjorg lasjorg left a comment

Choose a reason for hiding this comment

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

Formatting nitpicks.

Co-authored-by: Lasse Jørgensen <28780271+lasjorg@users.noreply.github.com>
@jdwilkin4 jdwilkin4 requested a review from lasjorg May 20, 2024 14:47
Copy link
Contributor

@lasjorg lasjorg left a comment

Choose a reason for hiding this comment

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

Don't really know where to put this, but the assert.equal(done, 0); on line 55 of step 77 should be a strictEqual so you can't pass with the string "0"

@lasjorg
Copy link
Contributor

lasjorg commented May 21, 2024

It did just dawn on me that switching the order of teaching else and else if might be an idea we should consider.

So teach if/else first, then introduce else if and put it in the middle of the if/else code from the previous step.

First if/else step

if (false) {
  console.log("If condition is true.");
} else {
  console.log("If condition is false.");
}

Then else if step

if (false) {
  console.log("If condition is true.");
} else if (true) {
  console.log("If condition is true.");
} else {
  console.log("If condition is false.");
}

That makes it more clear that else can be used with both if and else if


Anyway, I can approve this and we can revisit it later if needed.

Copy link
Contributor

@lasjorg lasjorg left a comment

Choose a reason for hiding this comment

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

LGTM, but I think switching the order is something to consider.

Copy link
Member

@Ksound22 Ksound22 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jdwilkin4 jdwilkin4 requested a review from lasjorg May 21, 2024 17:40
@lasjorg
Copy link
Contributor

lasjorg commented May 21, 2024

I think the text makes it sound like multiple if statements are the same as a if/else if. It doesn't really convey how the condition of the if statement the else if is connected to relates. If you look at how MDN explains it they have a nested example.

if (condition1)
  statement1
else
  if (condition2)
    statement2
  else
    if (condition3)
      statement3

Maybe we can nest the new if statement inside the else block in the seed code and then have them move it out to be a else if

Seed code:

if ("") {
  console.log("Condition is true");
} else {
  if (5 < 10) {
    console.log("5 is less than 10");
  }
}

Solution:

if ("") {
  console.log("Condition is true");
} else if (5 < 10) {
  console.log("5 is less than 10");
}

This needs to be changed as well somehow.

But instead of using multiple if statements, you can use else if statements to check multiple conditions in a single block of code.

So, maybe something like:

In the code, an new if statement with the condition of 5 < 10 has been nested inside the else block.

But instead of using nested if and else statements, you can use the else if statement.

@jdwilkin4
Copy link
Contributor Author

I have reverted the changes back the way it was before the if, else if switch.
I think it would be better to iron this approach out more because anytime we ask campers to move code around, it needs to be super clear in the seed code where to move it. And the hint text needs to be super clear too.

I vote we merge this in as is.
Then open a separate issue to hash out the details for the if else if stuff.
Then a new PR can be opened to iterate off of this

@lasjorg
Copy link
Contributor

lasjorg commented May 22, 2024

Sure, we can do that.

Anyway, here is one way we might write the instructions.

Move the nested if statement out of the else block and combine the two so it creates a else if block. You can look at the example code for help with the syntax.

Copy link
Contributor

@lasjorg lasjorg left a comment

Choose a reason for hiding this comment

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

Didn't test it, but if this just reverts to what it was before, then I already approved that.

@naomi-lgbt naomi-lgbt merged commit 2baa9e1 into freeCodeCamp:main May 23, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new javascript course These are for issues dealing with the new JS curriculum scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add step to teach else if statements in pyramid project
5 participants