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.

Sign up for GitHub

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

Jump to bottom

fix grammar issues and update tests in step 38 of music player project #53700

Closed
jdwilkin4 opened this issue Feb 14, 2024 · 4 comments · Fixed by #54632
Closed

fix grammar issues and update tests in step 38 of music player project #53700

jdwilkin4 opened this issue Feb 14, 2024 · 4 comments · Fixed by #54632
Labels
help wanted Open for all. You do not need permission to work on these. 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. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.

Comments

@jdwilkin4
Copy link
Contributor

jdwilkin4 commented Feb 14, 2024 •
edited

Describe the Issue

There are a few grammar issues in this description. Plus the description can be broken up with spacing a little bit better for readability.

updated description

Before playing the song, you need to make sure it starts from the beginning. This can be achieved by the use of the `currentTime` property on the `audio` object. 

Add an `if` statement to check whether the `userData?.currentSong` is falsy *OR* if `userData?.currentSong.id` is strictly not equal to `song.id`. This condition will check if no current song is playing or if the current song is different from the one that is about to be played.

Inside your `if` block, set the `audio.currentTime` property to `0`.

Also the tests are looking for a specific answer of userData?.currentSong === null || userData?.currentSong.id !== song.id for the condition.
But I don't see why we need to specifically check for userData?.currentSong === null when either of these options would be valid

!userData?.currentSong || userData?.currentSong.id !== song.id

userData?.currentSong === null || userData?.currentSong.id !== song.id

userData?.currentSong === undefined || userData?.currentSong.id !== song.id

The tests should be more flexible to allow any valid answer.

Also, order shouldn't matter here because we want to test that campers understand the concept. Not forcing a particular order here.

This should pass but currently doesn't

if(userData?.currentSong.id !== song.id||userData?.currentSong === null)

Affected Page

https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures-v8/learn-basic-string-and-array-methods-by-building-a-music-player/step-38

Your code

see explanation above

Expected behavior

see explanation above

Screenshots

No response

System

  • Device: [e.g. iPhone 6, Laptop]
  • OS: [e.g. iOS 14, Windows 10, Ubuntu 20.04]
  • Browser: [e.g. Chrome, Safari]
  • Version: [e.g. 22]

Additional context

No response
@jdwilkin4 jdwilkin4 added type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. first timers only Open for anyone doing contributions for first time. You do not need permission to work on these. new javascript course These are for issues dealing with the new JS curriculum help wanted Open for all. You do not need permission to work on these. and removed first timers only Open for anyone doing contributions for first time. You do not need permission to work on these. labels Feb 14, 2024
@aaqib605 aaqib605 mentioned this issue Feb 18, 2024
Closed
4 tasks
This was referenced Feb 28, 2024
Closed
Closed
@gikf
Copy link
Member

gikf commented Mar 4, 2024

Also, order shouldn't matter here because we want to test that campers understand the concept. Not forcing a particular order here.

This should pass but currently doesn't

if(userData?.currentSong.id !== song.id||userData?.currentSong === null)

There's slight issue with this. If the concept in this step is using the ||, in a way where order doesn't matter, this is not the best example.

userData?.currentSong.id !== song.id||userData?.currentSong === null
  • Second part is obsolete, there's no way that userData?.currentSong === null could be true when it is reached in this order. It will always be false.
    • If userData?.currentSong.id !== song.id is true, then second part is skipped by definition.
    • If userData?.currentSong.id !== song.id is false, because it didn't throw (see below) then userData.currentSong.id must exists, therefore userData?.currentSong === null cannot be true
userData?.currentSong === null || userData?.currentSong.id !== song.id
  • It can throw error. The point of using || in the condition above (where second condition depends on the first one) is that the second part is checked only when first part is false. In this case it means that userData.currentSong is not null, so it is possible to check what is the value of userData.currentSong.id. Otherwise, when userData.currentSong is null, userData?.currentSong.id will throw error:
Uncaught TypeError: Cannot read properties of null (reading 'id')
  • What about userData?.currentSong?.id !== song.id||userData?.currentSong === null? (notice the second question mark added in the first part). Well... the second condition is then again obsolete.

@jdwilkin4
Copy link
Contributor Author

@gikf

If order does matter then the directions need to specify that. Otherwise, campers will not understand why this should be allowed to pass

if(userData?.currentSong.id !== song.id||userData?.currentSong === null)

either an update needs to be to the directions on hint text. Or both.

@gikf
Copy link
Member

gikf commented Mar 7, 2024

Add an if statement to check whether the userData?.currentSong is falsy OR if userData?.currentSong.id is strictly not equal to song.id. This condition will check if no current song is playing or if the current song is different from the one that is about to be played.
Despite of using the logical OR, order in this statement is important, as userData?.currentSong.id depends on the existence of userData?.currentSong.

What do you think?

@jdwilkin4
Copy link
Contributor Author

jdwilkin4 commented Mar 7, 2024 •
edited

I like it.

I would just change the first part to this

Despite the use of the logical _OR_ operator,

@jdwilkin4 jdwilkin4 changed the title fix grammar issues and update tests in step 29 of music player project fix grammar issues and update tests in step 38 of music player project Apr 17, 2024
@Supravisor Supravisor mentioned this issue Apr 26, 2024
Closed
4 tasks
Supravisor added a commit to Supravisor/freeCodeCamp that referenced this issue Apr 28, 2024
@Supravisor
fix(curriculum): update grammar and tests music 38
bb13f06 
Checklist:

<!-- Please follow this checklist and put an x in each of the boxes, like this: [x]. It will ensure that our team takes your pull request seriously. -->

- [x] I have read and followed the [contribution guidelines](https://contribute.freecodecamp.org).
- [x] I have read and followed the [how to open a pull request guide](https://contribute.freecodecamp.org/#/how-to-open-a-pull-request).
- [x] My pull request targets the `main` branch of freeCodeCamp.
- [x] I have tested these changes either locally on my machine, or GitPod.

<!--If your pull request closes a GitHub issue, replace the XXXXX below with the issue number.-->

Closes freeCodeCamp#53700

<!-- Feel free to add any additional description of changes below this line -->
This was referenced Apr 28, 2024
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Open for all. You do not need permission to work on these. 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. type: bug Issues that need priority attention. Platform, Curriculum tests (if broken completely), etc.
Projects
None yet
Milestone
No milestone
2 participants
@gikf @jdwilkin4