Paths

Table of Contentst

Differential D8590

[native] Warn the user when deleting a chat that has contained chats
ClosedPublic
Actions

Authored by • patryk on Jul 21 2023, 7:22 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Thu, Jul 4, 12:11 PM

	Unknown Object (File)
	Mon, Jul 1, 3:17 AM

	Unknown Object (File)
	Mon, Jul 1, 2:08 AM

	Unknown Object (File)
	Sat, Jun 29, 4:32 PM

	Unknown Object (File)
	Fri, Jun 28, 9:19 PM

	Unknown Object (File)
	Mon, Jun 24, 2:42 PM

	Unknown Object (File)
	Sat, Jun 15, 8:01 PM

	Unknown Object (File)
	Fri, Jun 14, 7:39 AM

View All 79 Files

Subscribers

Details

Reviewers

bartek
tomek
inka
ashoat

Commits

rCOMMa071dedf22ea: [native] Warn the user when deleting a chat that has contained chats

Summary

Part of ENG-4319.

Before deletion, the user is warned about possible contained chats. For subchannel deletion, the modal looks like this:
iOS:

131415.png (956×453 px, 136 KB)

Andriod:

456.png (718×340 px, 60 KB)

When deleting a whole community, message changes:
iOS:

101112.png (956×453 px, 138 KB)

Android:

123.png (718×340 px, 60 KB)

When there are no contained chats, the confirmation modal does not pop up.

Depends on D8526.

Test Plan

Same as D8530 test plan.

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• patryk created this revision.Jul 21 2023, 7:22 AM

• patryk held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 21 2023, 7:22 AM

Harbormaster completed remote builds in B21113: Diff 28930.Jul 21 2023, 7:40 AM

• patryk edited the summary of this revision. (Show Details)Jul 21 2023, 7:41 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added reviewers: bartek, tomek, inka.

• patryk added a parent revision: D8526: [lib] Add containedThreadInfos selector.

• patryk attached a referenced file: F648099: 131415.png. (Show Details)

• patryk attached a referenced file: F648100: 456.png. (Show Details)

• patryk attached a referenced file: F648101: 101112.png. (Show Details)

• patryk attached a referenced file: F648102: 123.png. (Show Details)

• patryk published this revision for review.Jul 21 2023, 7:50 AM

Subtask to ENG-4423: ENG-4442

inka added inline comments.Jul 25 2023, 4:19 AM

native/chat/settings/delete-thread.react.js
146 ↗	(On Diff #28930)	This shouldn't say community when you are deleting a normal chat

This code has a lot of issues

native/chat/settings/delete-thread.react.js
62 ↗	(On Diff #28930)	Why is this `?boolean`? In what scenario will it be `null` or `undefined`?
77 ↗	(On Diff #28930)	This is not idiomatic React. I'm not sure if `this.props` is even set when this is called Even if it is set, your approach here is based on the value at construction, which may change later. Instead, you should implement a function `getDeletionTargetText` that checks the value at invocation time
143 ↗	(On Diff #28930)	Why the `?` after `this.props`? I'm pretty sure `this.props` is always set inside a React class component
143–155 ↗	(On Diff #28930)	You can reduce indentation and simplify the code here. See here
146 ↗	(On Diff #28930)	I agree – see comment here
284 ↗	(On Diff #28930)	This condition appears to technically be correct, but `undefined > 0` is not a very "readable" condition... most JS developers have no idea how this evaluates. Can you rework the condition so it's more clear?

This revision now requires changes to proceed.Jul 27 2023, 1:47 PM

• patryk edited the summary of this revision. (Show Details)Jul 31 2023, 3:32 AM

• patryk added a parent revision: D8586: [native] Use currently selected theme for alerts..

• patryk added inline comments.

native/chat/settings/delete-thread.react.js
62 ↗	(On Diff #28930)	I initially intended to make this prop optional in case someone wanted to reuse this component elsewhere. However, this approach may be slightly over-engineered -> will remove it in a next diff.

Rework code, use new Alert wrapper.

Harbormaster completed remote builds in B21357: Diff 29257.Jul 31 2023, 3:57 AM

Accepting, but please address inline comment before landing

native/chat/settings/delete-thread.react.js
142–148 ↗	(On Diff #29257)	This appears to be the same as on `web`. Please factor this out instead of using copy-paste. You should endeavor to use copy-paste as little as possible (ideally never do it)

This revision is now accepted and ready to land.Jul 31 2023, 10:34 AM

Use getThreadsToDeleteText function from lib.

Harbormaster completed remote builds in B21484: Diff 29477.Aug 2 2023, 4:06 AM

Closed by commit rCOMMa071dedf22ea: [native] Warn the user when deleting a chat that has contained chats (authored by • patryk). · Explain WhyAug 3 2023, 12:03 PM

This revision was automatically updated to reflect the committed changes.

• patryk added a commit: rCOMMa071dedf22ea: [native] Warn the user when deleting a chat that has contained chats.

Revision Contents
Changeset List

Path

Size

native/

chat/

settings/

delete-thread.react.js

40 lines

Diff 29519

native/chat/settings/delete-thread.react.js

Loading...