[native] Handle unsaved changes when 'cancel' is pressed
ClosedPublic
Actions

Authored by rohan on Jun 9 2023, 11:36 AM.

Details

Reviewers

atul
ginsu
tomek

Commits

rCOMM1dae2e974e2c: [native] Handle unsaved changes when 'cancel' is pressed

Summary

As a second part to the header left button, we want to ensure that a user is prompted with an alert warning them that they are discarding changes if they've selected a different role but not saved.

Depends on D8158

Test Plan

Please see the video below

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jun 9 2023, 11:36 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJun 9 2023, 11:36 AM

rohan edited the test plan for this revision. (Show Details)Jun 9 2023, 11:38 AM

rohan attached a referenced file: F580243: Simulator Screen Recording - iPhone 14 Pro - 2023-06-09 at 14.36.51.mp4. (Show Details)

rohan added a child revision: D8160: [native] Create a ChangeRolesHeaderRightButton to save role changes.Jun 9 2023, 11:42 AM

Harbormaster completed remote builds in B20138: Diff 27588.Jun 9 2023, 11:53 AM

rohan requested review of this revision.Jun 9 2023, 11:53 AM

rohan added a subscriber: • ted.Jun 9 2023, 12:12 PM

rohan added inline comments.

native/roles/change-roles-header-left-button.react.js
32–41 ↗	(On Diff #27588)	I think the wording here may be a bit confusing. In my experience, 'cancel' typically means close the alert, and there's another option like 'confirm' or something that will mean you're ok with discarding changes. Here it seems like 'cancel' means we're ok with discarding changes. Not sure if it's just me though (cc @ted)

ashoat added inline comments.Jun 9 2023, 12:25 PM

native/roles/change-roles-header-left-button.react.js
32–41 ↗	(On Diff #27588)	What do we do in the current edit-message workflow? Heads-up I generally handle copy, not @ted

rohan added inline comments.Jun 9 2023, 12:36 PM

native/roles/change-roles-header-left-button.react.js
32–41 ↗	(On Diff #27588)	Ah my bad. It looks like in message editing: `Continue editing` closes the alert and continues editing while `Discard edit` closes the alert and exits editing

ashoat added inline comments.Jun 9 2023, 12:48 PM

native/roles/change-roles-header-left-button.react.js
32–41 ↗	(On Diff #27588)	I agree we shouldn't use the term `Cancel` here... how about `Leave`? I don't think we need `{ cancelable: true }`... we can just default to `Stay` if the user dismisses the `Alert` Can we try to match copy for title / description with the edit-message workflow?

rohan added inline comments.Jun 9 2023, 1:01 PM

native/roles/change-roles-header-left-button.react.js
32–41 ↗	(On Diff #27588)	Yeah that makes sense, I'll update the diff

Requesting changes for changes suggested by @ashoat.

It's not clear to me as of this diff how the user is supposed to save their changes, maybe that's more clear in a subsequent diff?

This revision now requires changes to proceed.Jun 12 2023, 1:12 PM

In D8159#241874, @atul wrote:

It's not clear to me as of this diff how the user is supposed to save their changes, maybe that's more clear in a subsequent diff?

Yeah changes are handled with the header right button in D8160