London | 26-ITP-Sep | Ebrahim Moqbel | Sprint 2 | Coursework - #1467
Ebrahim-Moqbel wants to merge 23 commits into
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Good job! A few bits needs another look but its getting there! |
|
Hi @webmonch, I have answered the Questions and updated the answers accordingly Thank you for your valuable suggestions I believe it is ready to be reviewed now. |
There was a problem hiding this comment.
Most of the earlier comments are sorted, thanks. Your answer to f) in 2-time-format.js is a really good one.
A few things still to do:
-
2-mandatory-errors/4.jshas not been started. It is the one file in that folder you have not touched, and it still stops with an error. -
3-mandatory-interpret/1-percentage-change.jsanswer d), see my comment. -
1-key-exercises/4-random.jsline 11, see my comment. -
3-mandatory-interpret/2-time-format.jsanswer c), see my comment. -
2-mandatory-errors/1.js, see my comment. -
Could you run Prettier on the files you have changed? Ten of them are not formatted at the moment.
One more thing, and this one does not block the PR. The first four commits on this branch are from January, about Wireframe/articles.md. They are on your fork's main, so every branch you make from it picks them up. That is why the bot kept flagging this PR, and your Sprint 3 PR has the same thing. To clear it, sync your fork's main so it matches CodeYourFuture's main exactly, rather than merging into it. On GitHub, open your fork, click Sync fork, and if it offers "Discard commits" take that option. Worth doing before you start Sprint 4, if you haven't already.
Add the Needs Review label again when you have pushed.
abdishakoor-dev
left a comment
There was a problem hiding this comment.
Four of the six are done, thanks. The explanation in 1.js is accurate, and the rename in 4.js runs.
Three things left:
-
2-mandatory-errors/4.js, see my comment. -
3-mandatory-interpret/2-time-format.jsline 36, see my comment. -
Formatting. "My code is consistently formatted" is on your PR checklist, and the tool that does it for you is called Prettier. It rearranges spacing and indentation to one agreed style, so that your code is easy to read and so that a reviewer only sees the changes you meant to make, not stray spaces and tabs. Eleven of your twelve files fail that check at the moment.
Prettier comes with the CYF extension pack you were asked to install during onboarding. If you are not sure you have it, open VS Code, go to Extensions, and search for CodeYourFuture Extension Pack; install it if it is not there: https://marketplace.visualstudio.com/items?itemName=CodeYourFuture.cyf-extension-pack
Then open each of your Sprint-2 files, right click in the editor, choose Format Document, and pick Prettier if VS Code asks which formatter to use. Save, commit the changes it makes, and push. To make this happen every time you save, follow the format on save steps here: https://github.com/CodeYourFuture/Module-JavaScript-Fundamentals/blob/main/practical_guide.md
Add the Needs Review label again when you have pushed.
| @@ -1,2 +1,5 @@ | |||
| const 12HourClockTime = "8:53pm"; | |||
| const 24hourClockTime = "20:53"; | |||
| const twelveHourClockTime = "8:53pm"; | |||
There was a problem hiding this comment.
The rename is right and the file runs now. Section 2 of the Sprint 2 README also asks you to interpret the error and explain why it happened, and there is nothing written down in this file. Put the two original lines back for a moment, run node 4.js, and write down what node prints and why it happens. You can delete the log on line 5 once you have checked it.
| /*whole positive value: for the 8784 seconds the formatted duration would be fine 2:26:24 and the code would be working as expected | ||
| under 60 seconds: 59 seconds would assign the hours and minutes both as 0s resulting 0:0:59 instead of the normal formatted way 00:00:59 but still correct | ||
| zero value: the formatted duration would also be correct 0:0:0 even though it is an unusual value it still show that it can handle it | ||
| Negative value: for the -8784 seconds would give us an output of -2:-26:-29 which is not a valid format and doesn't make any sense |
There was a problem hiding this comment.
You changed the positive case on line 33 to 8784, good. This line still carries the seconds from the old number. What does the program print for -8784?
Learners, PR Template
Self checklist
Task code
CYF-1039
Changelist
I have met the requirements and specifications of the following files :
key-exercises
mandatory errors
mandatory interpret
Questions
N/A