Page MenuHomePhabricator
Differential D8139

[web] Confirm a user wants to exit if changes are made in the modal
ClosedPublic
Actions

Authored by rohan on Jun 7 2023, 5:06 AM.
Tags
None
Referenced Files
F3387411: D8139.id27581.diff
Fri, Nov 29, 9:21 AM
Unknown Object (File)
Mon, Nov 25, 10:28 PM
Unknown Object (File)
Mon, Nov 25, 9:52 PM
Unknown Object (File)
Mon, Nov 25, 9:36 PM
Unknown Object (File)
Mon, Nov 18, 10:28 AM
Unknown Object (File)
Mon, Nov 11, 11:11 AM
Unknown Object (File)
Mon, Nov 11, 3:22 AM
Unknown Object (File)
Sat, Nov 9, 1:25 PM
View All Files
Subscribers
ashoat
• ted
tomek

Details

Reviewers
atul
ginsu
Commits
rCOMMcbd9d2b8b5fe: [web] Confirm a user wants to exit if changes are made in the modal
Summary

Based on the designs, if a member's role is changed (not saved, but just different to their current role) in the dropdown menu, we need to add a confirmation exit modal to warn them that they will
lose their changes by closing the role change modal.

https://linear.app/comm/issue/ENG-4039/confirm-a-user-wants-to-exit-if-changes-are-made-in-the-modal

Depends on D8138

Test Plan

Please see the video below that covers the different use cases of no changes, changes, making a change but then changing the role back to the member's current role, etc.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan created this revision.Jun 7 2023, 5:06 AM
Herald added subscribers: • ted, tomek, ashoat. · View Herald TranscriptJun 7 2023, 5:06 AM
rohan added a child revision: D8140: [web] Add functionality on save to actually change a member's role.Jun 7 2023, 5:08 AM
rohan edited the test plan for this revision. (Show Details)Jun 7 2023, 5:23 AM
rohan attached a referenced file: F575232: Screen Recording 2023-06-07 at 8.22.09 AM.mov. (Show Details)
Harbormaster completed remote builds in B20081: Diff 27516.Jun 7 2023, 5:23 AM
rohan requested review of this revision.Jun 7 2023, 5:23 AM
rohan updated this revision to Diff 27546.Jun 8 2023, 6:11 AM

Rebase

Harbormaster completed remote builds in B20103: Diff 27546.Jun 8 2023, 6:27 AM
rohan updated this revision to Diff 27581.Jun 9 2023, 11:14 AM

Rebase on D8136

Harbormaster completed remote builds in B20131: Diff 27581.Jun 9 2023, 11:31 AM
atul accepted this revision.Jun 9 2023, 3:25 PM
atul added inline comments.
web/modals/unsaved-changes-modal.react.js
15–20 ↗(On Diff #27581)

Looks strange, but also reasonable?

This revision is now accepted and ready to land.Jun 9 2023, 3:25 PM
rohan updated this revision to Diff 27940.Jun 21 2023, 8:05 AM

Rebase

Harbormaster completed remote builds in B20386: Diff 27940.Jun 21 2023, 8:35 AM
Closed by commit rCOMMcbd9d2b8b5fe: [web] Confirm a user wants to exit if changes are made in the modal (authored by rohan). · Explain WhyJun 21 2023, 8:41 AM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMMcbd9d2b8b5fe: [web] Confirm a user wants to exit if changes are made in the modal.
ashoat added inline comments.Jun 27 2023, 10:31 AM
web/modals/threads/members/change-member-role-modal.react.js
50

It appears there there are other ways to close the modal that won't trigger UnsavedChangesModal. Is this addressed in later diffs?

rohan added inline comments.Jun 27 2023, 11:04 AM
web/modals/threads/members/change-member-role-modal.react.js
50

It's not, I hadn't considered the default modal close. I think the easiest solution is to get rid of popModal and have Modal onClose={onBackClick} (just off the top of my head, will test it before I put up a quick diff to address this)

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
members/
18 lines
27 lines
53 lines
1 line

Diff 27959

View Options

web/modals/threads/members/change-member-role-modal.react.js

Loading...
View Options

web/modals/unsaved-changes-modal.css

Loading...
View Options

web/modals/unsaved-changes-modal.react.js

Loading...
View Options

web/theme.css

Loading...