-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Release 2] [Domain Control] Close domain member account #78002
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
Changes from all commits
105f463
cae53c0
29a1b2a
c911a12
395608f
2006575
0a94209
5044ec0
953c767
2df3b19
7a5b50d
23a4ee9
b572b62
2b9b0ee
4ca3eac
58b92f7
c538487
0f41e23
c999814
44549c1
c41472d
ed8b441
377c136
a86bfdc
b5bb171
cef1fb3
167e2cd
1835e11
4658537
60b407a
c12825b
6e714fe
5bbc20d
177e059
fe9a617
eacb1ed
7492a30
8c9015f
94178b6
dc2ab86
b2b6491
5528ce0
e8f6e50
e3ee397
e632d5e
f8d979f
2e3d016
8248a9a
139ff4e
b36d8bb
a72ca30
56e84b1
f7f431d
ebd5b5f
14a9a5f
4c3abb5
5567acb
28eeff2
f52e6b0
be02ad7
2201b2b
85f7a16
6011cde
89f9077
350144e
85a9bb5
7767275
8173c34
3ce78ff
4b4d31d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,8 @@ import CONST from '@src/CONST'; | |
| import Button from './Button'; | ||
| import Header from './Header'; | ||
| import Modal from './Modal'; | ||
| import Text from './Text'; | ||
| import RenderHTML from './RenderHTML'; | ||
| import ScrollView from './ScrollView'; | ||
|
|
||
| type DecisionModalProps = { | ||
| /** Title describing purpose of modal */ | ||
|
|
@@ -20,6 +21,18 @@ type DecisionModalProps = { | |
| /** Text content used in second button */ | ||
| secondOptionText: string; | ||
|
|
||
| /** Whether the first option uses a success-themed button */ | ||
| isFirstOptionSuccess?: boolean; | ||
|
|
||
| /** Whether the second option uses a success-themed button */ | ||
| isSecondOptionSuccess?: boolean; | ||
|
|
||
| /** Whether the first option uses a danger-themed button */ | ||
| isFirstOptionDanger?: boolean; | ||
|
|
||
| /** Whether the second option uses a danger-themed button */ | ||
| isSecondOptionDanger?: boolean; | ||
|
|
||
| /** onSubmit callback fired after clicking on first button */ | ||
| onFirstOptionSubmit?: () => void; | ||
|
|
||
|
|
@@ -32,11 +45,29 @@ type DecisionModalProps = { | |
| /** Callback for closing modal */ | ||
| onClose: () => void; | ||
|
|
||
| /** Callback when modal has fully disappeared */ | ||
| onModalHide?: () => void; | ||
|
|
||
| /** Whether modal is visible */ | ||
| isVisible: boolean; | ||
| }; | ||
|
|
||
| function DecisionModal({title, prompt = '', firstOptionText, secondOptionText, onFirstOptionSubmit, onSecondOptionSubmit, isSmallScreenWidth, onClose, isVisible}: DecisionModalProps) { | ||
| function DecisionModal({ | ||
| title, | ||
| prompt = '', | ||
| firstOptionText, | ||
| secondOptionText, | ||
| onFirstOptionSubmit, | ||
| onSecondOptionSubmit, | ||
| isSmallScreenWidth, | ||
| onClose, | ||
| onModalHide, | ||
| isVisible, | ||
| isFirstOptionDanger = false, | ||
| isFirstOptionSuccess = true, | ||
| isSecondOptionSuccess = false, | ||
| isSecondOptionDanger = false, | ||
| }: DecisionModalProps) { | ||
| const styles = useThemeStyles(); | ||
|
|
||
| return ( | ||
|
|
@@ -45,21 +76,23 @@ function DecisionModal({title, prompt = '', firstOptionText, secondOptionText, o | |
| isVisible={isVisible} | ||
| type={isSmallScreenWidth ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.CONFIRM} | ||
| innerContainerStyle={styles.pv0} | ||
| onModalHide={onModalHide} | ||
| > | ||
| <View style={[styles.m5]}> | ||
| <ScrollView contentContainerStyle={styles.m5}> | ||
| <View> | ||
| <View style={[styles.flexRow, styles.mb5]}> | ||
| <Header | ||
| title={title} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-3 (docs)Minor: Inconsistent style array usage. Lines 75 and 79 wrap single styles in arrays Suggested fix: <Header
title={title}
containerStyles={styles.alignItemsCenter}
/>
```\n\n---\n\nPlease rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| containerStyles={[styles.alignItemsCenter]} | ||
| containerStyles={styles.alignItemsCenter} | ||
| /> | ||
| </View> | ||
| <Text>{prompt}</Text> | ||
| <RenderHTML html={prompt} /> | ||
| </View> | ||
| {!!firstOptionText && ( | ||
| <Button | ||
| success | ||
| style={[styles.mt5]} | ||
| success={isFirstOptionSuccess} | ||
| danger={isFirstOptionDanger} | ||
| style={styles.mt5} | ||
| onPress={onFirstOptionSubmit} | ||
| pressOnEnter | ||
| text={firstOptionText} | ||
|
|
@@ -70,9 +103,11 @@ function DecisionModal({title, prompt = '', firstOptionText, secondOptionText, o | |
| style={[firstOptionText ? styles.mt3 : styles.mt5, styles.noSelect]} | ||
| onPress={onSecondOptionSubmit} | ||
| text={secondOptionText} | ||
| success={isSecondOptionSuccess} | ||
| danger={isSecondOptionDanger} | ||
| large | ||
| /> | ||
| </View> | ||
| </ScrollView> | ||
| </Modal> | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| type DeleteDomainMemberParams = { | ||
| targetEmail: string; | ||
| domain: string; | ||
| overrideProcessingReports: boolean; | ||
| }; | ||
|
|
||
| export default DeleteDomainMemberParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a good example what will soon be agains the new code rule in the App #80222 @sumo-slonik @war-in
Can you please take a look and work on approaching this with that rule in mind so we do not make it worse?
cc @adhorodyski @TMisiukiewicz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. What do you think about using a union type for button variants instead?
like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, does that actually follow the merit of the rule? it feels like the same problem just using a struct as a param instead of boolean flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny
Maybe we can slim down this component and go with something like this?
and usage like this:
I can migrate DecisionModal to this implementation in a separate PR before we merge, so we don’t mix these two changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we agree to go this route, I have a PR ready for the DecisionModal migration. We can either merge this one as is and update all DecisionModals (including this one) later, or run the migration first and then use the migrated version in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @ZhenjaHorbach @situchan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with it. If we wanna migrate first and then use migrated version, Release 2 for Domain Control will be held for long time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but lets follow up on this one! @sumo-slonik can you please create an issue for the PR you made with explanation of the motivation behind it?