Page MenuHomePhabricator
Differential D8159

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

Authored by rohan on Jun 9 2023, 11:36 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 21, 9:04 PM
Unknown Object (File)
Fri, Mar 21, 1:37 PM
Unknown Object (File)
Fri, Mar 21, 12:26 PM
Unknown Object (File)
Fri, Mar 21, 2:30 AM
Unknown Object (File)
Thu, Mar 20, 1:28 AM
Unknown Object (File)
Mon, Mar 3, 6:22 PM
Unknown Object (File)
Mon, Feb 24, 3:07 PM
Unknown Object (File)
Mon, Feb 24, 3:07 PM
View All Files
Subscribers
ashoat
• ted

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
Lint Not Applicable
Unit
Tests Not Applicable

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)
  1. I agree we shouldn't use the term Cancel here... how about Leave?
  2. I don't think we need { cancelable: true }... we can just default to Stay if the user dismisses the Alert
  3. 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

atul requested changes to this revision.Jun 12 2023, 1:12 PM

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
rohan added a comment.Jun 12 2023, 1:30 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

rohan updated this revision to Diff 27667.Jun 12 2023, 1:30 PM

Address feedback

rohan mentioned this in D8158: [native] Create a ChangeRolesHeaderLeftButton to cancel role changes.Jun 12 2023, 1:32 PM
Harbormaster completed remote builds in B20197: Diff 27667.Jun 12 2023, 1:49 PM
atul accepted this revision.Jun 13 2023, 6:42 PM
This revision is now accepted and ready to land.Jun 13 2023, 6:42 PM
ashoat added inline comments.Jun 15 2023, 6:18 AM
native/roles/change-roles-header-left-button.react.js
34 ↗(On Diff #27667)

Missing a period at the end

rohan updated this revision to Diff 27949.Jun 21 2023, 8:06 AM

Rebase

This revision was landed with ongoing or failed builds.Jun 21 2023, 8:42 AM
Closed by commit rCOMM1dae2e974e2c: [native] Handle unsaved changes when 'cancel' is pressed (authored by rohan). · Explain Why
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM1dae2e974e2c: [native] Handle unsaved changes when 'cancel' is pressed.
Harbormaster completed remote builds in B20395: Diff 27949.Jun 21 2023, 9:29 AM
ashoat added inline comments.Jun 28 2023, 2:51 PM
native/roles/change-roles-header-left-button.react.js
34

Still missing a period at the end

ashoat added inline comments.Jun 28 2023, 2:56 PM
native/roles/change-roles-header-left-button.react.js
34

Ah looks like it was addressed in D8277

Revision Contents
Changeset List

PathSize
native/
chat/
6 lines
roles/
38 lines

Diff 27968

View Options

native/chat/chat.react.js

Show First 20 Lines • Show All 135 Lines • ▼ Show 20 Linesconst light = Object.freeze({
subthreadsModalClose: designSystemColors.shadesBlack60, subthreadsModalClose: designSystemColors.shadesBlack60,
subthreadsModalBackground: designSystemColors.shadesWhite80, subthreadsModalBackground: designSystemColors.shadesWhite80,
subthreadsModalSearch: '#00000008', subthreadsModalSearch: '#00000008',
messageLabel: designSystemColors.shadesBlack100, messageLabel: designSystemColors.shadesBlack100,
modalSeparator: designSystemColors.shadesWhite60, modalSeparator: designSystemColors.shadesWhite60,
secondaryButtonBorder: designSystemColors.shadesWhite100, secondaryButtonBorder: designSystemColors.shadesWhite100,
inviteLinkLinkColor: designSystemColors.shadesBlack100, inviteLinkLinkColor: designSystemColors.shadesBlack100,
inviteLinkButtonBackground: designSystemColors.shadesWhite60, inviteLinkButtonBackground: designSystemColors.shadesWhite60,
emojiKeyboardKnob: designSystemColors.shadesWhite90,
emojiKeyboardCategoryIcon: designSystemColors.shadesBlack70,
emojiKeyboardContainerActive: designSystemColors.shadesWhite90,
ashoatUnsubmitted
Not Done
Inline Actions

Do we really need one-offs here? Why can't we find an existing color family that works with our designs?

We need to push the team (@tedchang in particular) to use existing families (and update them as necessary) instead of hacking around inconsistencies like this. When we take this sort of approach of picking colors on a per-screen basis, we fail to maintain stylistic / design consistency throughout the app

ashoat: Do we really need one-offs here? Why can't we find an existing color family that works with our…
ashoatUnsubmitted
Not Done
Inline Actions

Meant to tag @ted

ashoat: Meant to tag @ted
}); });
export type Colors = $Exact<typeof light>; export type Colors = $Exact<typeof light>;
const dark: Colors = Object.freeze({ const dark: Colors = Object.freeze({
blockQuoteBackground: '#A9A9A9', blockQuoteBackground: '#A9A9A9',
blockQuoteBorder: designSystemColors.shadesBlack60, blockQuoteBorder: designSystemColors.shadesBlack60,
codeBackground: designSystemColors.shadesBlack100, codeBackground: designSystemColors.shadesBlack100,
disabledButton: designSystemColors.shadesBlack80, disabledButton: designSystemColors.shadesBlack80,
▲ Show 20 Lines • Show All 81 Lines • ▼ Show 20 Linesconst dark: Colors = Object.freeze({
typeaheadTooltipBackground: '#1F1F1f', typeaheadTooltipBackground: '#1F1F1f',
typeaheadTooltipBorder: designSystemColors.shadesBlack80, typeaheadTooltipBorder: designSystemColors.shadesBlack80,
typeaheadTooltipText: 'white', typeaheadTooltipText: 'white',
messageLabel: designSystemColors.shadesWhite60, messageLabel: designSystemColors.shadesWhite60,
modalSeparator: designSystemColors.shadesBlack80, modalSeparator: designSystemColors.shadesBlack80,
secondaryButtonBorder: designSystemColors.shadesWhite100, secondaryButtonBorder: designSystemColors.shadesWhite100,
inviteLinkLinkColor: designSystemColors.shadesWhite80, inviteLinkLinkColor: designSystemColors.shadesWhite80,
inviteLinkButtonBackground: designSystemColors.shadesBlack80, inviteLinkButtonBackground: designSystemColors.shadesBlack80,
emojiKeyboardKnob: designSystemColors.shadesWhite100,
emojiKeyboardCategoryIcon: designSystemColors.shadesWhite70,
emojiKeyboardContainerActive: designSystemColors.shadesBlack60,
}); });
const colors = { light, dark }; const colors = { light, dark };
const colorsSelector: (state: AppState) => Colors = createSelector( const colorsSelector: (state: AppState) => Colors = createSelector(
(state: AppState) => state.globalThemeInfo.activeTheme, (state: AppState) => state.globalThemeInfo.activeTheme,
(theme: ?GlobalTheme) => { (theme: ?GlobalTheme) => {
const explicitTheme = theme ? theme : 'light'; const explicitTheme = theme ? theme : 'light';
return colors[explicitTheme]; return colors[explicitTheme];
▲ Show 20 Lines • Show All 127 Lines • Show Last 20 Lines
View Options

native/roles/change-roles-header-left-button.react.js

Show First 20 Lines • Show All 135 Lines • ▼ Show 20 Linesconst light = Object.freeze({
subthreadsModalClose: designSystemColors.shadesBlack60, subthreadsModalClose: designSystemColors.shadesBlack60,
subthreadsModalBackground: designSystemColors.shadesWhite80, subthreadsModalBackground: designSystemColors.shadesWhite80,
subthreadsModalSearch: '#00000008', subthreadsModalSearch: '#00000008',
messageLabel: designSystemColors.shadesBlack100, messageLabel: designSystemColors.shadesBlack100,
modalSeparator: designSystemColors.shadesWhite60, modalSeparator: designSystemColors.shadesWhite60,
secondaryButtonBorder: designSystemColors.shadesWhite100, secondaryButtonBorder: designSystemColors.shadesWhite100,
inviteLinkLinkColor: designSystemColors.shadesBlack100, inviteLinkLinkColor: designSystemColors.shadesBlack100,
inviteLinkButtonBackground: designSystemColors.shadesWhite60, inviteLinkButtonBackground: designSystemColors.shadesWhite60,
emojiKeyboardKnob: designSystemColors.shadesWhite90,
emojiKeyboardCategoryIcon: designSystemColors.shadesBlack70,
emojiKeyboardContainerActive: designSystemColors.shadesWhite90,
ashoatUnsubmitted
Not Done
Inline Actions

Do we really need one-offs here? Why can't we find an existing color family that works with our designs?

We need to push the team (@tedchang in particular) to use existing families (and update them as necessary) instead of hacking around inconsistencies like this. When we take this sort of approach of picking colors on a per-screen basis, we fail to maintain stylistic / design consistency throughout the app

ashoat: Do we really need one-offs here? Why can't we find an existing color family that works with our…
ashoatUnsubmitted
Not Done
Inline Actions

Meant to tag @ted

ashoat: Meant to tag @ted
}); });
export type Colors = $Exact<typeof light>; export type Colors = $Exact<typeof light>;
const dark: Colors = Object.freeze({ const dark: Colors = Object.freeze({
blockQuoteBackground: '#A9A9A9', blockQuoteBackground: '#A9A9A9',
blockQuoteBorder: designSystemColors.shadesBlack60, blockQuoteBorder: designSystemColors.shadesBlack60,
codeBackground: designSystemColors.shadesBlack100, codeBackground: designSystemColors.shadesBlack100,
disabledButton: designSystemColors.shadesBlack80, disabledButton: designSystemColors.shadesBlack80,
▲ Show 20 Lines • Show All 81 Lines • ▼ Show 20 Linesconst dark: Colors = Object.freeze({
typeaheadTooltipBackground: '#1F1F1f', typeaheadTooltipBackground: '#1F1F1f',
typeaheadTooltipBorder: designSystemColors.shadesBlack80, typeaheadTooltipBorder: designSystemColors.shadesBlack80,
typeaheadTooltipText: 'white', typeaheadTooltipText: 'white',
messageLabel: designSystemColors.shadesWhite60, messageLabel: designSystemColors.shadesWhite60,
modalSeparator: designSystemColors.shadesBlack80, modalSeparator: designSystemColors.shadesBlack80,
secondaryButtonBorder: designSystemColors.shadesWhite100, secondaryButtonBorder: designSystemColors.shadesWhite100,
inviteLinkLinkColor: designSystemColors.shadesWhite80, inviteLinkLinkColor: designSystemColors.shadesWhite80,
inviteLinkButtonBackground: designSystemColors.shadesBlack80, inviteLinkButtonBackground: designSystemColors.shadesBlack80,
emojiKeyboardKnob: designSystemColors.shadesWhite100,
emojiKeyboardCategoryIcon: designSystemColors.shadesWhite70,
emojiKeyboardContainerActive: designSystemColors.shadesBlack60,
}); });
const colors = { light, dark }; const colors = { light, dark };
const colorsSelector: (state: AppState) => Colors = createSelector( const colorsSelector: (state: AppState) => Colors = createSelector(
(state: AppState) => state.globalThemeInfo.activeTheme, (state: AppState) => state.globalThemeInfo.activeTheme,
(theme: ?GlobalTheme) => { (theme: ?GlobalTheme) => {
const explicitTheme = theme ? theme : 'light'; const explicitTheme = theme ? theme : 'light';
return colors[explicitTheme]; return colors[explicitTheme];
▲ Show 20 Lines • Show All 127 Lines • Show Last 20 Lines