Add generic AutoCompleteSuggestions component #18024
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
szebniok
force-pushed
the
rework-emoji-suggestions
branch
from
e0d9635 to
9e23a06
Compare
|
I have read the CLA Document and I hereby sign the CLA |
szebniok
force-pushed
the
rework-emoji-suggestions
branch
from
a05be2e to
125db68
Compare
szebniok
force-pushed
the
rework-emoji-suggestions
branch
from
125db68 to
f798983
Compare
| const propTypes = { | ||
| /** Array of suggestions */ | ||
| // eslint-disable-next-line react/forbid-prop-types | ||
| suggestions: PropTypes.arrayOf(PropTypes.object).isRequired, | ||
|
|
||
| /** Function used to render each suggestion, returned JSX will be enclosed inside a Pressable component */ | ||
| renderSuggestionMenuItem: PropTypes.func.isRequired, | ||
|
|
||
| /** Create unique keys for each suggestion item */ | ||
| keyExtractor: PropTypes.func.isRequired, | ||
|
|
||
| /** The index of the highlighted suggestion */ | ||
| highlightedSuggestionIndex: PropTypes.number.isRequired, | ||
|
|
||
| /** Fired when the user selects a suggestion */ | ||
| onSelect: PropTypes.func.isRequired, | ||
|
|
||
| /** Show that we can use large auto-complete suggestion picker. | ||
| * Depending on available space and whether the input is expanded, we can have a small or large mention suggester. | ||
| * When this value is false, the suggester will have a height of 2.5 items. When this value is true, the height can be up to 5 items. */ | ||
| isSuggestionPickerLarge: PropTypes.bool.isRequired, | ||
|
|
||
| /** Show that we should include ReportRecipientLocalTime view height */ | ||
| shouldIncludeReportRecipientLocalTimeHeight: PropTypes.bool.isRequired, | ||
| }; |
There was a problem hiding this comment.
I see all props are required. Can we make some of them optional? i.e. highlightedSuggestionIndex is fine not to be required. Before, the default value was 0
There was a problem hiding this comment.
I was considering making some of the props optional, but ultimately decided against it (YAGNI).
The way I saw it:
suggestions and renderSuggestionMenuItem - It doesn't make sense to use this component without providing the suggestions and a way of displaying them
keyExtractor - I could consider making it optional, as FlatList is falling back to using item.key, item.id and the item index
highlightedSuggestionIndex - I think that the default value of 0 is problematic—we either care about the highlights and set this value, or we don't keep track of it, but in this case we have an useless piece of data lying around.
onSelect - I initially rejected it, but now I can see a potential need of displaying the suggestions without a way of interacting with them
isSuggestionPickerLarge and shouldIncludeReportRecipientLocalTimeHeight - I would say that these props may be ugly, and are tightly coupling our component to the report composer, but they were necessary in the previous implementation.
The way I see it, I could consider making the keyExtractor and onSelect optional, and set the default value of highlightedSuggestionIndex to undefined.
There was a problem hiding this comment.
That sounds good to me 👍🏾
| <Text style={styles.emojiSuggestionsText}> | ||
| : | ||
| {_.map(styledTextArray, ({text, isColored}, i) => ( | ||
| <Text key={`${text}+${i}`} style={StyleUtils.getColoredBackgroundStyle(isColored)}> |
There was a problem hiding this comment.
This existed before but not sure why index is used for key. Is there any possibility that texts are duplicated?
There was a problem hiding this comment.
I don't think that is likely scenario, as the item.name is splitted into at most into 3 parts, and item.name contains the : sign at the beginning and at the end.
I could consider setting the key to text, but I could see an argument being made that the current solution is more future-proof.
There was a problem hiding this comment.
I feel like index is safer in general as a key.
|
@0xmiroslav looks like the comments we have here are minor. Would you be able to do the all-device testing today? @szebniok you have some conflicts again. |
|
Sure, will do once main merged into this branch |
|
@0xmiroslav @puneetlath I've merged the |
|
Great. @0xmiroslav can we get this tested today? |
|
|
||
| export default EmojiSuggestions; | ||
| // AutoCompleteSuggestions.defaultProps = defaultProps; |
There was a problem hiding this comment.
NAB: we can remove comment
|
|
||
| AutoCompleteSuggestions.propTypes = propTypes; | ||
|
|
||
| // AutoCompleteSuggestions.defaultProps = defaultProps; |
There was a problem hiding this comment.
NAB: we can remove comment
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
|
||
| export default EmojiSuggestions; | ||
| // AutoCompleteSuggestions.defaultProps = defaultProps; |
|
|
||
| AutoCompleteSuggestions.propTypes = propTypes; | ||
|
|
||
| // AutoCompleteSuggestions.defaultProps = defaultProps; |
| const propTypes = { | ||
| /** Array of suggestions */ | ||
| // eslint-disable-next-line react/forbid-prop-types | ||
| suggestions: PropTypes.arrayOf(PropTypes.object).isRequired, | ||
|
|
||
| /** Function used to render each suggestion, returned JSX will be enclosed inside a Pressable component */ | ||
| renderSuggestionMenuItem: PropTypes.func.isRequired, | ||
|
|
||
| /** Create unique keys for each suggestion item */ | ||
| keyExtractor: PropTypes.func.isRequired, | ||
|
|
||
| /** The index of the highlighted suggestion */ | ||
| highlightedSuggestionIndex: PropTypes.number.isRequired, | ||
|
|
||
| /** Fired when the user selects a suggestion */ | ||
| onSelect: PropTypes.func.isRequired, | ||
|
|
||
| /** Show that we can use large auto-complete suggestion picker. | ||
| * Depending on available space and whether the input is expanded, we can have a small or large mention suggester. | ||
| * When this value is false, the suggester will have a height of 2.5 items. When this value is true, the height can be up to 5 items. */ | ||
| isSuggestionPickerLarge: PropTypes.bool.isRequired, | ||
|
|
||
| /** Show that we should include ReportRecipientLocalTime view height */ | ||
| shouldIncludeReportRecipientLocalTimeHeight: PropTypes.bool.isRequired, | ||
| }; |
There was a problem hiding this comment.
That sounds good to me 👍🏾
| <Text style={styles.emojiSuggestionsText}> | ||
| : | ||
| {_.map(styledTextArray, ({text, isColored}, i) => ( | ||
| <Text key={`${text}+${i}`} style={StyleUtils.getColoredBackgroundStyle(isColored)}> |
There was a problem hiding this comment.
I feel like index is safer in general as a key.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.10-0 🚀
|
|
Can we check this off since another ticket in prod? #14686 (comment) |
|
@mvtglobally I don't follow the question. Mind clarifying? |
|
@puneetlath i am referring to QA steps |
|
Thanks for confirming. Yes we do need to test. We can test using the QA steps from #14686 |
|
Posting so team can reference QA Steps Mobile: Enter :s in an empty text input. Observe that there is no suggestion. Add 'm' to the text input so it contains :sm. Verify that the Emoji suggester appears above the text input. Tap on any of the suggested emojis. Verify that :sm is replaced with the selected emoji. Enter some text in the text input. Add a space or create a new line. Expand the input Enter a colon followed by two letters that correspond to the name of the desired emoji (e.g. smile -> :sm, flag -> :fl). Verify that the Emoji suggester appears above the text input. Tap on one of the suggested emojis. Verify that the text (e.g. :sm, :fl, etc.) is replaced with the selected emoji. WEB/Desktop: Enter :s in an empty text input. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
Details
I've created the
AutoCompleteSuggestionscomponent and rewritten theEmojiSuggestionsto use it. This is a first PR related to #17890—the second one will implement theMentionSuggestions, which will use theAutoCompleteSuggestions.Fixed Issues
$ #17890
Tests
Offline tests
QA Steps
This PR refactors functionality added in #14686, so the tests can be reused here.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-04-26.at.14.19.08.mov
Mobile Web - Chrome
Screen.Recording.2023-04-26.at.14.23.05.mov
Mobile Web - Safari
Screen.Recording.2023-04-26.at.14.32.14.mov
Desktop
Screen.Recording.2023-04-26.at.14.37.13.mov
iOS
Screen.Recording.2023-04-26.at.14.40.33.mov
Android
Screen.Recording.2023-04-26.at.14.50.12.mov