-
Notifications
You must be signed in to change notification settings - Fork 10
Etem-w3-JavaScript #20
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
base: main
Are you sure you want to change the base?
Conversation
### 1-JavaScript - Week3 | ||
|
||
| Exercise | Passed | Failed | ESLint | | ||
|----------------------------|--------|--------|--------| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I can't run your code. It is complaining about the branch name. The branch name is correct but a bit strange.
I got the following error when I run npm test
:
Your branch name does conform to the mandated pattern. Valid branch names should
start with your name, followed by the week number and the module name in this
format:
YOUR_NAME-w2-JavaScript
YOUR_NAME-w3-JavaScript
YOUR_NAME-w1-Browsers
YOUR_NAME-w1-UsingAPIs
YOUR_NAME-w2-UsingAPIs
For more information please refer to the link below:
https://github.com/HackYourFuture/JavaScript/blob/main/hand-in-assignments-guide.md#12-every-week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benetem What happens on your end if you run npm test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ddoyediran, this might happen if you check out a PR using the VSCode GitHub Pull Request extension. In this particular example it checks out the PR under the branch name pr/benetem/20
, which is of course not a compliant branch name for the test runner. You can disable the branch name check by renaming the file .env-example
to .env
. This latter file is untracked, so should not be affected by checking out subsequent PRs.
Hope this helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remarcmij Thank you so much for this nice info. Yes, it helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will proceed to approve now but you can try to update exercise 3 solution accordingly with the comments. Great job!!!
} | ||
} | ||
return newNumbers; | ||
return numbers.filter((num) => num % 2 === 0).map((num) => num * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct.
@@ -30,15 +30,18 @@ const mondayTasks = [ | |||
|
|||
const hourlyRate = 25; | |||
|
|||
function computeEarnings(/* TODO parameter(s) go here */) { | |||
// TODO complete this function | |||
function computeEarnings(tasks, hourlyRate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct
@@ -16,10 +16,10 @@ export function createObservable() { | |||
const subscribers = []; | |||
return { | |||
subscribe(subscriber) { | |||
// TODO complete this function | |||
subscribers.push(subscriber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct
@@ -75,7 +75,7 @@ const quiz = { | |||
b: 'cash, name', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also looks correct
// TODO replace next line with your code | ||
expect(false).toBe(true); | ||
console.log(sanitizeFruitBasket.length); | ||
expect(sanitizeFruitBasket).toHaveLength(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice implementation.
}); | ||
|
||
test('sanitizeFruitBasket should not modify the original `fruitBasket` array', () => { | ||
// Save the original contents of the fruit basket | ||
const fruitBasket = ['apple', 'banana', 'lemon', 'orange']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it works but we don't have to hard code it like this. We can remove line 40 and then simply check as follows:
...
const originalFruitBasketContents = [...fruitBasket];
expect(fruitBasket).toEqual(originalFruitBasketContents);
// TODO replace next line with your code | ||
expect(false).toBe(true); | ||
const sanitizedFruitBasket = sanitizeFruitBasket(fruitBasket, 'lemon'); | ||
expect(sanitizedFruitBasket).toEqual([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we don't have to hard code. We can simply check if 'lemon'
is not in the sanitizedFruitBasket
output array as follows:
...
expect(sanitizedFruitBasket).not.toContain('lemon');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback. I applied it based on your comment.
No description provided.