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.

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

feat(form-v2): instantiate emergency contact modal in AdminNavBar #4381

Merged
merged 10 commits into from
Aug 2, 2022

Conversation

wanlingt
Copy link
Contributor

@wanlingt wanlingt commented Jul 26, 2022

Problem

The emergency contact modal should be instantiated in the AdminNavBar instead of the WorkSpacePage

Builds upon #4125
Closes #4110 , #4153 , #4317

Solution

This PR instantiates the emergency contact modal in the AdminNavBar. The emergency contact modal pops up if:

  • The user clicks 'Contact' in the AdminNavBar dropdown menu
  • If the user does not have an existing emergency contact upon logging in

Details:

  • emergencyContactKey is deleted from localStorage upon logout

Breaking Changes

  • No - this PR is backwards compatible

Before & After Screenshots

AFTER:
chrome-capture-2022-6-27

Tests

In sequence

  • Log in. If rollout announcement and emergency contact keys exist in local storage, delete them. On the workspace page, the rollout announcement modal should appear first, then the emergency contact modal. Refresh the workspace page. Both modals should not pop up.
  • Click on 'Contact' in the Admin Navbar drop down menu. The emergency contact modal should appear. Fill in your contact number.
  • Log out. Check local storage - the emergency contact key should be cleared.
  • Log in again. the emergency contact modal should not pop up.

@wanlingt wanlingt marked this pull request as ready for review July 27, 2022 01:32
@wanlingt wanlingt requested review from r00dgirl, karrui and justynoh July 27, 2022 01:32
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
@wanlingt wanlingt requested a review from karrui July 27, 2022 06:16
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
@wanlingt wanlingt requested a review from karrui July 27, 2022 10:12
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

frontend/src/app/AdminNavBar/AdminNavBar.tsx Outdated Show resolved Hide resolved
// Remove logged in state from localStorage
localStorage.removeItem(LOGGED_IN_KEY)
// Event to let useLocalStorage know that key is being deleted.
window.dispatchEvent(new Event(LOCAL_STORAGE_EVENT))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/justANote:
Not from your PR, but the working of LOCAL_STORAGE_EVENT and useLocalStorage seems a bit funny and inefficient 🤔. Every component which use useLocalStorage (and specifies the key it cares about) take an action on all key changes. Storage events by right contain information about what changed, so listeners can take informed decisions.

Not a blocker to merge, maybe I'll have a chat with KarRui later.

@wanlingt wanlingt requested a review from karrui July 29, 2022 06:02
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@karrui
Copy link
Contributor

karrui commented Aug 2, 2022

might need to fix some stories, the workspace page stories are being hidden behind the modal.
could we set the modal to not show up on workspace page?

@wanlingt
Copy link
Contributor Author

wanlingt commented Aug 2, 2022

might need to fix some stories, the workspace page stories are being hidden behind the modal. could we set the modal to not show up on workspace page?

as discussed, the emergency contact modal doesnt show up bc the mock user already has a contact number, but the rollout announcement modal still appears. will open a separate issue for this #4460

@wanlingt wanlingt merged commit 0af2c11 into form-v2/develop Aug 2, 2022
@wanlingt wanlingt deleted the form-v2/admin-navbar-emergency-contact branch August 2, 2022 04:53
@justynoh justynoh mentioned this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants