-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Simplify Global Create menu #25564
Simplify Global Create menu #25564
Changes from all commits
65edf96
d0ef97a
3e61212
31ba218
8e26741
d0362c8
0e46476
725ae24
cb8d751
6bcc194
cd2780a
2e5f42e
b3fa521
70a7032
45d606a
79a02d5
cdcea69
94b8c0d
eb01b1f
a221a57
fe065c1
166a27a
58cd065
3fb9ec0
bad65d4
90afb07
e4348e1
9b3c9af
afb89eb
ad56566
0f1648d
b7907a5
8ad5472
d2a2e50
a2e3350
d63e67c
fb2e415
3826fcb
85519f7
45085a5
df48bd8
8a1cf12
ccd86bd
8ebe2a6
02df4e3
602ba38
22234f4
2cd6be3
058ecfb
cc67090
5b8e6a8
2ee0a42
5788291
5d28fdb
70beac8
0d2d560
7ff6e00
f45b2ca
90b96de
4c478d3
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 |
---|---|---|
|
@@ -2,7 +2,7 @@ import _ from 'underscore'; | |
import lodashGet from 'lodash/get'; | ||
import React, {Component} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import {View, InteractionManager} from 'react-native'; | ||
import {View} from 'react-native'; | ||
import Button from '../Button'; | ||
import FixedFooter from '../FixedFooter'; | ||
import OptionsList from '../OptionsList'; | ||
|
@@ -16,11 +16,9 @@ import KeyboardShortcut from '../../libs/KeyboardShortcut'; | |
import {propTypes as optionsSelectorPropTypes, defaultProps as optionsSelectorDefaultProps} from './optionsSelectorPropTypes'; | ||
import setSelection from '../../libs/setSelection'; | ||
import compose from '../../libs/compose'; | ||
import getPlatform from '../../libs/getPlatform'; | ||
|
||
const propTypes = { | ||
/** Whether we should wait before focusing the TextInput, useful when using transitions on Android */ | ||
shouldDelayFocus: PropTypes.bool, | ||
|
||
/** padding bottom style of safe area */ | ||
safeAreaPaddingBottomStyle: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), | ||
|
||
|
@@ -70,6 +68,12 @@ class BaseOptionsSelector extends Component { | |
componentDidMount() { | ||
this.subscribeToKeyboardShortcut(); | ||
|
||
if (this.props.isFocused && this.props.autoFocus && this.textInput) { | ||
setTimeout(() => { | ||
this.textInput.focus(); | ||
}, CONST.ANIMATED_TRANSITION); | ||
} | ||
|
||
this.scrollToIndex(this.props.selectedOptions.length ? 0 : this.state.focusedIndex, false); | ||
} | ||
|
||
|
@@ -82,12 +86,13 @@ class BaseOptionsSelector extends Component { | |
} | ||
} | ||
|
||
if (this.textInput && this.props.autoFocus && !prevProps.isFocused && this.props.isFocused) { | ||
InteractionManager.runAfterInteractions(() => { | ||
// If we automatically focus on a text input when mounting a component, | ||
// let's automatically focus on it when the component updates as well (eg, when navigating back from a page) | ||
// Screen coming back into focus, for example | ||
// when doing Cmd+Shift+K, then Cmd+K, then Cmd+Shift+K. | ||
// Only applies to platforms that support keyboard shortcuts | ||
if ([CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform()) && !prevProps.isFocused && this.props.isFocused && this.props.autoFocus && this.textInput) { | ||
setTimeout(() => { | ||
this.textInput.focus(); | ||
}); | ||
}, CONST.ANIMATED_TRANSITION); | ||
} | ||
|
||
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. We cannot simply remove this code. This breaks auto focus in android. 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. We might consider using 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.
Let's follow this pattern Rory suggested
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. That pattern was not followed correctly. |
||
if (_.isEqual(this.props.sections, prevProps.sections)) { | ||
|
@@ -359,8 +364,6 @@ class BaseOptionsSelector extends Component { | |
selectTextOnFocus | ||
blurOnSubmit={Boolean(this.state.allOptions.length)} | ||
spellCheck={false} | ||
autoFocus={this.props.autoFocus} | ||
shouldDelayFocus={this.props.shouldDelayFocus} | ||
/> | ||
); | ||
const optionsList = ( | ||
|
@@ -372,6 +375,9 @@ class BaseOptionsSelector extends Component { | |
focusedIndex={this.state.focusedIndex} | ||
selectedOptions={this.props.selectedOptions} | ||
canSelectMultipleOptions={this.props.canSelectMultipleOptions} | ||
shouldShowMultipleOptionSelectorAsButton={this.props.shouldShowMultipleOptionSelectorAsButton} | ||
multipleOptionSelectorButtonText={this.props.multipleOptionSelectorButtonText} | ||
onAddToSelection={this.props.onAddToSelection} | ||
hideSectionHeaders={this.props.hideSectionHeaders} | ||
headerMessage={this.props.headerMessage} | ||
boldStyle={this.props.boldStyle} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,15 @@ const propTypes = { | |
/** Whether we can select multiple options */ | ||
canSelectMultipleOptions: PropTypes.bool, | ||
|
||
/** Whether to show a button pill instead of a standard tickbox */ | ||
shouldShowMultipleOptionSelectorAsButton: PropTypes.bool, | ||
|
||
/** Text for button pill */ | ||
multipleOptionSelectorButtonText: PropTypes.string, | ||
|
||
/** Callback to fire when the multiple selector (tickbox or button) is clicked */ | ||
onAddToSelection: PropTypes.func, | ||
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. Same here 😄 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. I was just about pointing this out :D |
||
|
||
Comment on lines
+56
to
+64
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. These are missing in defaultProps |
||
/** Whether we highlight selected options */ | ||
highlightSelectedOptions: PropTypes.bool, | ||
|
||
|
@@ -125,6 +134,9 @@ const defaultProps = { | |
selectedOptions: [], | ||
headerMessage: '', | ||
canSelectMultipleOptions: false, | ||
shouldShowMultipleOptionSelectorAsButton: false, | ||
multipleOptionSelectorButtonText: '', | ||
onAddToSelection: () => {}, | ||
highlightSelectedOptions: false, | ||
hideSectionHeaders: false, | ||
boldStyle: false, | ||
|
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.
Wrapping with
PressableWithFeedback
here caused super minor inconsistency - Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split BillWe should have passed
disabled={isDisabled}
.