-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Limit tag list to 500 items #31447
Limit tag list to 500 items #31447
Changes from 17 commits
c570773
8e0ab70
f12d261
a5c9de5
5d07521
c8928e7
653deac
b95cf0e
a820d4e
ffd144e
82c9f1c
a1a62c7
0bcc5a9
09e72c7
e2d780d
19ca4ac
2cde176
c29740a
19ff8d8
059a302
8f952bc
1de984d
0f794ec
b08423a
9520db9
3234a9b
96c23a4
f99086a
49154ff
b4d1537
1080ca0
c4f1f63
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 |
---|---|---|
|
@@ -27,17 +27,16 @@ import * as IOU from '@userActions/IOU'; | |
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import ROUTES from '@src/ROUTES'; | ||
import Button from './Button'; | ||
import ButtonWithDropdownMenu from './ButtonWithDropdownMenu'; | ||
import categoryPropTypes from './categoryPropTypes'; | ||
import ConfirmedRoute from './ConfirmedRoute'; | ||
import FormHelpMessage from './FormHelpMessage'; | ||
import * as Expensicons from './Icon/Expensicons'; | ||
import Image from './Image'; | ||
import MenuItemWithTopDescription from './MenuItemWithTopDescription'; | ||
import optionPropTypes from './optionPropTypes'; | ||
import OptionsSelector from './OptionsSelector'; | ||
import SettlementButton from './SettlementButton'; | ||
import ShowMore from './ShowMore'; | ||
import Switch from './Switch'; | ||
import tagPropTypes from './tagPropTypes'; | ||
import Text from './Text'; | ||
|
@@ -635,20 +634,10 @@ function MoneyRequestConfirmationList(props) { | |
numberOfLinesTitle={2} | ||
/> | ||
{!shouldShowAllFields && ( | ||
<View style={[styles.flexRow, styles.justifyContentBetween, styles.mh3, styles.alignItemsCenter, styles.mb2, styles.mt1]}> | ||
<View style={[styles.shortTermsHorizontalRule, styles.flex1, styles.mr0]} /> | ||
<Button | ||
small | ||
onPress={toggleShouldExpandFields} | ||
text={translate('common.showMore')} | ||
shouldShowRightIcon | ||
iconRight={Expensicons.DownArrow} | ||
iconFill={theme.icon} | ||
style={styles.mh0} | ||
/> | ||
|
||
<View style={[styles.shortTermsHorizontalRule, styles.flex1, styles.ml0]} /> | ||
</View> | ||
<ShowMore | ||
containerStyle={styles.mt1} | ||
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. Bottom margin missed here which caused #33758 |
||
onPress={toggleShouldExpandFields} | ||
/> | ||
)} | ||
{shouldShowAllFields && ( | ||
<> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ import Icon from '@components/Icon'; | |||||
import {Info} from '@components/Icon/Expensicons'; | ||||||
import OptionsList from '@components/OptionsList'; | ||||||
import {PressableWithoutFeedback} from '@components/Pressable'; | ||||||
import ShowMore from '@components/ShowMore'; | ||||||
import Text from '@components/Text'; | ||||||
import TextInput from '@components/TextInput'; | ||||||
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize'; | ||||||
|
@@ -74,6 +75,7 @@ class BaseOptionsSelector extends Component { | |||||
this.selectFocusedOption = this.selectFocusedOption.bind(this); | ||||||
this.addToSelection = this.addToSelection.bind(this); | ||||||
this.updateSearchValue = this.updateSearchValue.bind(this); | ||||||
this.incrementPage = this.incrementPage.bind(this); | ||||||
this.relatedTarget = null; | ||||||
|
||||||
const allOptions = this.flattenSections(); | ||||||
|
@@ -85,6 +87,7 @@ class BaseOptionsSelector extends Component { | |||||
shouldDisableRowSelection: false, | ||||||
shouldShowReferralModal: false, | ||||||
errorMessage: '', | ||||||
paginationPage: 1, | ||||||
}; | ||||||
} | ||||||
|
||||||
|
@@ -189,8 +192,43 @@ class BaseOptionsSelector extends Component { | |||||
return defaultIndex; | ||||||
} | ||||||
|
||||||
/** | ||||||
* When pagination is enabled, | ||||||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* maps sections to render only allowed count of them. | ||||||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
*/ | ||||||
get sections() { | ||||||
if (!_.isNumber(this.props.itemsPerPage)) { | ||||||
return this.props.sections; | ||||||
} | ||||||
|
||||||
return _.map(this.props.sections, (section) => { | ||||||
if (_.isEmpty(section.data)) { | ||||||
return section; | ||||||
} | ||||||
|
||||||
return { | ||||||
...section, | ||||||
data: section.data.slice(0, this.props.itemsPerPage * this.state.paginationPage), | ||||||
}; | ||||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Calculates all exactly visible options of sections. | ||||||
rezkiy37 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
*/ | ||||||
get allVisibleOptionsCount() { | ||||||
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 shouldn't use new code patterns without getting a buy-in from most of the team because it can reduce readability. 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. Got you, I will update it to methods. 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. Done - 059a302. |
||||||
let count = 0; | ||||||
|
||||||
_.forEach(this.sections, (section) => { | ||||||
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. did you mean i can't find
Suggested change
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. Yes, it is the 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. Done - 059a302. |
||||||
count += lodashGet(section, 'data.length', 0); | ||||||
}); | ||||||
|
||||||
return count; | ||||||
} | ||||||
|
||||||
updateSearchValue(value) { | ||||||
this.setState({ | ||||||
paginationPage: 1, | ||||||
errorMessage: value.length > this.props.maxLength ? this.props.translate('common.error.characterLimitExceedCounter', {length: value.length, limit: this.props.maxLength}) : '', | ||||||
}); | ||||||
|
||||||
|
@@ -387,7 +425,17 @@ class BaseOptionsSelector extends Component { | |||||
this.props.onAddToSelection(option); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Increments a pagination page to show more items | ||||||
*/ | ||||||
incrementPage() { | ||||||
this.setState((prev) => ({ | ||||||
paginationPage: prev.paginationPage + 1, | ||||||
})); | ||||||
} | ||||||
|
||||||
render() { | ||||||
const shouldShowShowMore = this.state.allOptions.length > this.props.itemsPerPage * this.state.paginationPage; | ||||||
const shouldShowFooter = | ||||||
!this.props.isReadOnly && (this.props.shouldShowConfirmButton || this.props.footerContent) && !(this.props.canSelectMultipleOptions && _.isEmpty(this.props.selectedOptions)); | ||||||
const defaultConfirmButtonText = _.isUndefined(this.props.confirmButtonText) ? this.props.translate('common.confirm') : this.props.confirmButtonText; | ||||||
|
@@ -424,7 +472,7 @@ class BaseOptionsSelector extends Component { | |||||
ref={(el) => (this.list = el)} | ||||||
optionHoveredStyle={this.props.optionHoveredStyle} | ||||||
onSelectRow={this.props.onSelectRow ? this.selectRow : undefined} | ||||||
sections={this.props.sections} | ||||||
sections={this.sections} | ||||||
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. Same - #31447 (comment). 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. Done - 059a302. |
||||||
focusedIndex={this.state.focusedIndex} | ||||||
selectedOptions={this.props.selectedOptions} | ||||||
canSelectMultipleOptions={this.props.canSelectMultipleOptions} | ||||||
|
@@ -458,6 +506,16 @@ class BaseOptionsSelector extends Component { | |||||
shouldPreventDefaultFocusOnSelectRow={this.props.shouldPreventDefaultFocusOnSelectRow} | ||||||
nestedScrollEnabled={this.props.nestedScrollEnabled} | ||||||
bounces={!this.props.shouldTextInputAppearBelowOptions || !this.props.shouldAllowScrollingChildren} | ||||||
renderFooterContent={() => | ||||||
shouldShowShowMore && ( | ||||||
<ShowMore | ||||||
containerStyle={styles.mv2} | ||||||
currentCount={this.props.itemsPerPage * this.state.paginationPage} | ||||||
totalCount={this.state.allOptions.length} | ||||||
onPress={this.incrementPage} | ||||||
/> | ||||||
) | ||||||
} | ||||||
/> | ||||||
); | ||||||
|
||||||
|
@@ -475,7 +533,7 @@ class BaseOptionsSelector extends Component { | |||||
<ArrowKeyFocusManager | ||||||
disabledIndexes={this.disabledOptionsIndexes} | ||||||
focusedIndex={this.state.focusedIndex} | ||||||
maxIndex={this.state.allOptions.length - 1} | ||||||
maxIndex={this.allVisibleOptionsCount - 1} | ||||||
onFocusedIndexChanged={this.props.disableArrowKeysActions ? () => {} : this.updateFocusedIndex} | ||||||
shouldResetIndexOnEndReached={false} | ||||||
> | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import PropTypes from 'prop-types'; | ||
import React from 'react'; | ||
import {Text, View} from 'react-native'; | ||
import _ from 'underscore'; | ||
import Button from '@components/Button'; | ||
import * as Expensicons from '@components/Icon/Expensicons'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import * as NumberFormatUtils from '@libs/NumberFormatUtils'; | ||
import stylePropTypes from '@styles/stylePropTypes'; | ||
import styles from '@styles/styles'; | ||
import themeColors from '@styles/themes/default'; | ||
|
||
const propTypes = { | ||
/** Additional styles for container */ | ||
containerStyle: stylePropTypes, | ||
|
||
/** A number of current showed items */ | ||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
currentCount: PropTypes.number, | ||
|
||
/** A number of total items */ | ||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
totalCount: PropTypes.number, | ||
|
||
/** A handler fires when button has been pressed */ | ||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
onPress: PropTypes.func.isRequired, | ||
}; | ||
|
||
const defaultProps = { | ||
containerStyle: {}, | ||
currentCount: undefined, | ||
totalCount: undefined, | ||
}; | ||
|
||
function ShowMore({containerStyle, currentCount, totalCount, onPress}) { | ||
puneetlath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const {translate, preferredLocale} = useLocalize(); | ||
|
||
const shouldShowCounter = _.isNumber(currentCount) && _.isNumber(totalCount); | ||
|
||
return ( | ||
<View style={[styles.alignItemsCenter, containerStyle]}> | ||
{shouldShowCounter && ( | ||
<Text style={[styles.mb2, styles.textLabelSupporting]}> | ||
{`${translate('common.showing')} `} | ||
<Text style={styles.textStrong}>{currentCount}</Text> | ||
{` ${translate('common.of')} `} | ||
<Text style={styles.textStrong}>{NumberFormatUtils.format(preferredLocale, totalCount)}</Text> | ||
</Text> | ||
)} | ||
<View style={[styles.w100, styles.flexRow, styles.justifyContentBetween, styles.alignItemsCenter]}> | ||
<View style={[styles.shortTermsHorizontalRule, styles.flex1, styles.mr0]} /> | ||
<Button | ||
style={styles.mh0} | ||
small | ||
shouldShowRightIcon | ||
iconFill={themeColors.icon} | ||
iconRight={Expensicons.DownArrow} | ||
text={translate('common.showMore')} | ||
accessibilityLabel={translate('common.showMore')} | ||
onPress={onPress} | ||
/> | ||
<View style={[styles.shortTermsHorizontalRule, styles.flex1, styles.ml0]} /> | ||
</View> | ||
</View> | ||
); | ||
} | ||
|
||
ShowMore.displayName = 'ShowMore'; | ||
ShowMore.propTypes = propTypes; | ||
ShowMore.defaultProps = defaultProps; | ||
|
||
export default ShowMore; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import PropTypes from 'prop-types'; | ||
import tagPropTypes from '@components/tagPropTypes'; | ||
import safeAreaInsetPropTypes from '@pages/safeAreaInsetPropTypes'; | ||
|
||
const propTypes = { | ||
/** The policyID we are getting tags for */ | ||
|
@@ -23,6 +24,9 @@ const propTypes = { | |
|
||
/** Should show the selected option that is disabled? */ | ||
shouldShowDisabledAndSelectedOption: PropTypes.bool, | ||
|
||
/** Safe area insets required for mobile devices margins */ | ||
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 think we can modify this comment to explain why and how we should use this required prop. 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. It is simple insets from safe area. It is a common RN feature. Anyway, I will try to describe comments. 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. Done - 19ff8d8. |
||
insets: safeAreaInsetPropTypes.isRequired, | ||
}; | ||
|
||
const defaultProps = { | ||
|
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 think we should set this max for anything that uses this optionList component - categories, tags, etc. Not just tags.
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.
So, you mean to set this limit by default? Right now it works like a property we can pass.
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.
That's what I was thinking yeah. Though I'm not sure all the places this component is used, so it'd be good to sanity-check that it makes sense.
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 can find all places and will post it here.
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.
So, @puneetlath, the component is BaseOptionsSelector and being used for 8 cases: Category Picker, Tag Picker, Money Request participant, Split Bill participant, Search, Start Chat. Task Assign, Task Share.
Screenshoots
Obviously, I've tried to test the limitation. Actually, I like how it works for all cases.
Note: for this video I've changed the limitation to 10 items per page. You can also set any value.
Example.mp4
Added a commit - b08423a.
cc @rushatgabhane
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.
And it'll be nice that any field that uses the options selector in the future, will automatically have this "show more" button built in. I don't even think we need to make it a prop. We could just make it a feature of that component. If in the future someone wants to make it configurable, then they can add it as a prop.
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.
Exactly, agree.
So, updating... Thank you for quick feedback 😉
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.
Done - 96c23a4.
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 love the consistency in behavior across all these screens! 🙌
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.
Looks like we are all aligned, so waiting on the review. @rushatgabhane, your turn 😉