-
Notifications
You must be signed in to change notification settings - Fork 19
Halyna R. #14
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?
Halyna R. #14
Conversation
remarcmij
left a comment
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 @halyna1995, something strange happened with the code of task-2. However, it works fine just the same. I have some other minor comments but will approve your PR now.
| setupEventListeners(); | ||
|
|
||
| export { errorMessage, successMessage }; | ||
| export { showMessage }; |
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'm not sure what happened. This looks like an older version of the assignment that Stas has changed since. Did you make these changes?
task-2/login.js
Outdated
| function errorMessage(message) { | ||
| showMessage(message, "error"); | ||
| } | ||
|
|
||
| function successMessage(message) { | ||
| showMessage(message, "success"); | ||
| } |
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.
These functions were supposed to be in the app.js file which was not be touched.
task-2/login.js
Outdated
|
|
||
| // after increment, if now blocked -> show blocked (requirement 3+4) | ||
| if (incorrectAttempts >= 4) { | ||
| errorMessage("Login blocked: Too many incorrect attempts", "error"); |
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.
Your errorMessage() function takes only one parameter.
| ); | ||
| } | ||
| } else { | ||
| console.log("Invalid selection. Please choose either 1 or 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.
If you add an option 3, it should also be reflected in the error message.
| } else if (menuSelection === "1") { | ||
| // EUR -> USD | ||
| const eurAmountInput = prompt("Enter amount in EUR: "); | ||
| const eurAmountNum = Number(eurAmountInput); | ||
|
|
||
| if (Number.isNaN(eurAmountNum) || eurAmountNum <= 0) { | ||
| console.log("Please enter a valid positive number for the amount."); | ||
| } else { |
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.
You already handled option 1 in the code above.
Reset incorrect attempts on successful login.
Co-authored-by: Jim Cramer <remarcmij@gmail.com>
remarcmij
left a comment
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 @halyna1995, it looks like I approved too soon.
| @@ -1,12 +1,34 @@ | |||
| // Do not change the line below | |||
| import { errorMessage, successMessage } from './app.js'; | |||
| import { showMessage } from './app.js'; | |||
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.
Have you tried running this code in the browser? It no longer runs because you now need to import errorMessage and successMessage from app.js.
| if (isValid) { | ||
| successMessage("Logged in successfully"); | ||
| incorrectAttempts = 0; | ||
| incorrectAttempts = 0; |
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.
Why do you have the same line duplicated here?
Removed redundant reset of incorrectAttempts.
Import success and error message functions from app.js.
No description provided.