Added profile page by gabito1451 · Pull Request #9 · gabito1451/mini-bank-app-code-Javascript

gabito1451 · 2023-02-20T19:50:40Z

Added profile page

nwankwo-ikemefuna

More work required

nwankwo-ikemefuna · 2023-02-22T20:54:16Z

profile.html

+          <input
+            type="checkbox"
+            id="change-pin"
+            onclick="displayFormToUpdate()"


displayFormToUpdate() does not describe what this checkbox does. How about togglePinChangeSectionDisplay(). The idea is that when the checkbox is checked, the PIN change section will show, otherwise it will be hidden.

Also, it's advised to attach an "onchange" event listener to the element instead of directly adding the function to onclick attribute. This is to separate html and javascript properly and give you more control.

nwankwo-ikemefuna · 2023-02-22T21:13:57Z

profile.js

+  let userToBeUpdated = allUsers.find(
+    (user) => user.accountNumber == accountNumberElem.value
+  );
+  userToBeUpdated.accountName = accountNameElem.value;
+  userToBeUpdated.accountPin = newPinElem.value;
+  // changing the updated properties to a string
+  const updatedObject = JSON.stringify(allUsers);
+  //  storing it back in local storage
+  localStorage.setItem("MB_USER_ACCOUNTS", updatedObject);


You're not doing it right here. You haven't added the modified user to the users array. You're simply pushing back the same users array to local storage.

See sample of how to update a user's details in deposit.js:

See how it's done on line 31.

Suggested pseudocode:

const currentUserIndex = getUserIndexByAccountNumber(currentUserAccountNumber); allUsers[currentUserIndex].accountName = the updated account name; // You need to check that user is updating PIN before you update it, otherwise, it will be updated to empty string if (user is also changing PIN) { allUsers[currentUserIndex].accountPin = the updated account PIN; } setLocalStorageArrData("MB_USER_ACCOUNTS", allUsers);

nwankwo-ikemefuna · 2023-02-22T21:15:52Z

profile.html

+            <input
+              type="password"
+              id="new-pin"
+              required


Changing PIN should be optional. You should programmatically add the required attribute if the checkbox is checked, and remove it otherwise. As it is now, you can't submit the form unless the checkbox is checked.

nwankwo-ikemefuna · 2023-02-22T21:16:01Z

profile.html

+            <input
+              type="password"
+              id="confirm-new-pin"
+              required


Changing PIN should be optional. You should programmatically add the required attribute if the checkbox is checked, and remove it otherwise. As it is now, you can't submit the form unless the checkbox is checked.

nwankwo-ikemefuna · 2023-02-22T21:16:51Z

profile.html

+            <input
+              type="password"
+              id="current-pin"
+              required


Changing PIN should be optional. You should programmatically add the required attribute if the checkbox is checked, and remove it otherwise. As it is now, you can't submit the form unless the checkbox is checked.

nwankwo-ikemefuna · 2023-02-22T21:19:17Z

profile.js

+updateBtnElem.addEventListener("click", () => updateForm());
+
+function deleteUserAccount() {
+  const registeredUsers = localStorage.getItem("MB_USER_ACCOUNTS");


Don't we have a function for getting all users defined in common.js? Why do this?

nwankwo-ikemefuna · 2023-02-22T21:19:57Z

profile.js

+function deleteUserAccount() {
+  const registeredUsers = localStorage.getItem("MB_USER_ACCOUNTS");
+  console.log(registeredUsers);
+  let usersArray = JSON.parse(registeredUsers);


What is your justification for using let here instead of const?

I've changed it since i am nor re-assigning any variable to a new value

nwankwo-ikemefuna · 2023-02-22T21:21:48Z

profile.js

+  let usersArray = JSON.parse(registeredUsers);
+  const index = getUserIndexByAccountNumber(currentUserAccountNumber);
+  usersArray.splice(index, 1);
+  const newArrayOfUsers = JSON.stringify(usersArray);


JSON.stringify(usersArray) will produce a string, so calling it newArrayOfUsers is misleading.

nwankwo-ikemefuna · 2023-02-22T21:22:36Z

profile.js

+}
+updateBtnElem.addEventListener("click", () => updateForm());
+
+function deleteUserAccount() {


There should be confirm action incase the user clicks the button by error.

Isn't closeUserAccount()a more appropriate function name?

nwankwo-ikemefuna · 2023-02-22T21:27:31Z

profile.html

+        <li><a href="#">Account Profile</a></li>
+        <li><a id="logout-btn" href="#">Logout</a></li>
+      </ul>
+    </nav>


Where's the account balance section?

I've fixed it

nwankwo-ikemefuna · 2023-02-22T21:37:23Z

More general comments:

When I visit the profile page, I want my name to be populated in the Account Name field.
If I don't interact with the "Change PIN" checkbox, I should be able to update only my account name.
The "Change PIN" checkbox is meant to be a toggle for the PIN section (i.e. hide or show the PIN section). Currently, the checkbox only works the first time to show the PIN section and subsequently doesn't affect the display of the PIN section. This gives an unpleasant experience.

nwankwo-ikemefuna · 2023-02-22T21:46:03Z

profile.js

+
+  // finding the current user to update profile
+
+  let userToBeUpdated = allUsers.find(


What is your justification for using let here instead of const?

Added profile page

e642dc0

gabito1451 requested a review from nwankwo-ikemefuna February 20, 2023 19:50

nwankwo-ikemefuna requested changes Feb 22, 2023

View reviewed changes

nwankwo-ikemefuna reviewed Feb 22, 2023

View reviewed changes

made the necessary corrections

92cfad5


		// finding the current user to update profile

		let userToBeUpdated = allUsers.find(

Conversation

gabito1451 commented Feb 20, 2023

Uh oh!

nwankwo-ikemefuna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwankwo-ikemefuna commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nwankwo-ikemefuna Feb 22, 2023 •

edited

Loading

nwankwo-ikemefuna Feb 22, 2023 •

edited

Loading

nwankwo-ikemefuna Feb 22, 2023 •

edited

Loading

nwankwo-ikemefuna Feb 22, 2023 •

edited

Loading

nwankwo-ikemefuna Feb 22, 2023 •

edited

Loading