From f6addb491b4312215e30511c5eca2087a346c9c3 Mon Sep 17 00:00:00 2001 From: Nev Angelova Date: Sun, 8 Dec 2019 14:10:56 +0100 Subject: [PATCH] [GH-11437] Fix thumbs emoji sorting logic for mobile (#3633) * [GH-11437] Fix thumbs emoji sorting logic for mobile * Add unit tests * Sort emojis in selector and handle string emojis * Remove redundant serverVersion logic * Fix typo in app/utils/emoji_utils.test.js Co-Authored-By: Saturnino Abril --- .../emoji_suggestion/emoji_suggestion.js | 37 +---- .../autocomplete/emoji_suggestion/index.js | 4 +- .../emoji_picker/emoji_picker_base.js | 16 +-- app/components/emoji_picker/index.js | 1 - app/utils/emoji_utils.js | 39 ++++++ app/utils/emoji_utils.test.js | 131 +++++++++++++++++- 6 files changed, 183 insertions(+), 45 deletions(-) diff --git a/app/components/autocomplete/emoji_suggestion/emoji_suggestion.js b/app/components/autocomplete/emoji_suggestion/emoji_suggestion.js index 6e43bae95..a8e3cfa9e 100644 --- a/app/components/autocomplete/emoji_suggestion/emoji_suggestion.js +++ b/app/components/autocomplete/emoji_suggestion/emoji_suggestion.js @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {Component} from 'react'; +import React, {PureComponent} from 'react'; import PropTypes from 'prop-types'; import { FlatList, @@ -10,8 +10,6 @@ import { View, } from 'react-native'; -import {isMinimumServerVersion} from 'mattermost-redux/utils/helpers'; - import AutocompleteDivider from 'app/components/autocomplete/autocomplete_divider'; import Emoji from 'app/components/emoji'; import TouchableWithFeedback from 'app/components/touchable_with_feedback'; @@ -22,7 +20,7 @@ import {makeStyleSheetFromTheme} from 'app/utils/theme'; const EMOJI_REGEX = /(^|\s|^\+|^-)(:([^:\s]*))$/i; const EMOJI_REGEX_WITHOUT_PREFIX = /\B(:([^:\s]*))$/i; -export default class EmojiSuggestion extends Component { +export default class EmojiSuggestion extends PureComponent { static propTypes = { actions: PropTypes.shape({ addReactionToLatestPost: PropTypes.func.isRequired, @@ -38,7 +36,6 @@ export default class EmojiSuggestion extends Component { onResultCountChange: PropTypes.func.isRequired, rootId: PropTypes.string, value: PropTypes.string, - serverVersion: PropTypes.string, nestedScrollEnabled: PropTypes.bool, }; @@ -80,35 +77,15 @@ export default class EmojiSuggestion extends Component { const oldMatchTerm = this.matchTerm; this.matchTerm = match[3] || ''; - // If we're server version 4.7 or higher - if (isMinimumServerVersion(this.props.serverVersion, 4, 7)) { - if (this.matchTerm !== oldMatchTerm && this.matchTerm.length) { - this.props.actions.autocompleteCustomEmojis(this.matchTerm); - return; - } - - if (this.matchTerm.length) { - this.handleFuzzySearch(this.matchTerm, nextProps); - } else { - const initialEmojis = [...nextProps.emojis]; - initialEmojis.splice(0, 300); - const data = initialEmojis.sort(); - - this.setEmojiData(data); - } - + if (this.matchTerm !== oldMatchTerm && this.matchTerm.length) { + this.props.actions.autocompleteCustomEmojis(this.matchTerm); return; } - // If we're server version 4.6 or lower - if (this.matchTerm !== oldMatchTerm) { + if (this.matchTerm.length) { this.handleFuzzySearch(this.matchTerm, nextProps); - } else if (!this.matchTerm.length) { - const initialEmojis = [...nextProps.emojis]; - initialEmojis.splice(0, 300); - const data = initialEmojis.sort(); - - this.setEmojiData(data); + } else { + this.setEmojiData(nextProps.emojis); } } diff --git a/app/components/autocomplete/emoji_suggestion/index.js b/app/components/autocomplete/emoji_suggestion/index.js index c6404202b..8738247d4 100644 --- a/app/components/autocomplete/emoji_suggestion/index.js +++ b/app/components/autocomplete/emoji_suggestion/index.js @@ -11,6 +11,7 @@ import {autocompleteCustomEmojis} from 'mattermost-redux/actions/emojis'; import {addReactionToLatestPost} from 'app/actions/views/emoji'; import {getTheme} from 'mattermost-redux/selectors/entities/preferences'; import {EmojiIndicesByAlias} from 'app/utils/emojis'; +import {compareEmojis} from 'app/utils/emoji_utils'; import EmojiSuggestion from './emoji_suggestion'; import Fuse from 'fuse.js'; @@ -23,7 +24,7 @@ const getEmojisByName = createSelector( emoticons.add(key); } - return Array.from(emoticons); + return Array.from(emoticons).sort(compareEmojis); } ); @@ -45,7 +46,6 @@ function mapStateToProps(state) { fuse, emojis, theme: getTheme(state), - serverVersion: state.entities.general.serverVersion, }; } diff --git a/app/components/emoji_picker/emoji_picker_base.js b/app/components/emoji_picker/emoji_picker_base.js index 58d6f4e30..f9889696c 100644 --- a/app/components/emoji_picker/emoji_picker_base.js +++ b/app/components/emoji_picker/emoji_picker_base.js @@ -16,8 +16,6 @@ import { import FontAwesomeIcon from 'react-native-vector-icons/FontAwesome'; import sectionListGetItemLayout from 'react-native-section-list-get-item-layout'; -import {isMinimumServerVersion} from 'mattermost-redux/utils/helpers'; - import Emoji from 'app/components/emoji'; import FormattedText from 'app/components/formatted_text'; import {DeviceTypes} from 'app/constants'; @@ -50,7 +48,6 @@ export default class EmojiPicker extends PureComponent { fuse: PropTypes.object.isRequired, isLandscape: PropTypes.bool.isRequired, onEmojiPress: PropTypes.func, - serverVersion: PropTypes.string, theme: PropTypes.object.isRequired, actions: PropTypes.shape({ getCustomEmojis: PropTypes.func.isRequired, @@ -87,13 +84,13 @@ export default class EmojiPicker extends PureComponent { filteredEmojis: [], searchTerm: '', currentSectionIndex: 0, - missingPages: isMinimumServerVersion(this.props.serverVersion, 4, 7), + missingPages: true, }; } - componentWillReceiveProps(nextProps) { + componentDidUpdate(prevProps) { this.rebuildEmojis = false; - if (this.props.deviceWidth !== nextProps.deviceWidth) { + if (this.props.deviceWidth !== prevProps.deviceWidth) { this.rebuildEmojis = true; if (this.searchBarRef) { @@ -101,7 +98,7 @@ export default class EmojiPicker extends PureComponent { } } - if (this.props.emojis !== nextProps.emojis) { + if (this.props.emojis !== prevProps.emojis) { this.rebuildEmojis = true; this.setRebuiltEmojis(); } @@ -190,9 +187,6 @@ export default class EmojiPicker extends PureComponent { clearTimeout(this.searchTermTimeout); const timeout = searchTerm ? 100 : 0; this.searchTermTimeout = setTimeout(async () => { - if (isMinimumServerVersion(this.props.serverVersion, 4, 7)) { - await this.props.actions.searchCustomEmojis(searchTerm); - } const filteredEmojis = this.searchEmojis(searchTerm); this.setState({ filteredEmojis, @@ -308,7 +302,7 @@ export default class EmojiPicker extends PureComponent { }; loadMoreCustomEmojis = async () => { - if (!this.props.customEmojisEnabled || !isMinimumServerVersion(this.props.serverVersion, 4, 7)) { + if (!this.props.customEmojisEnabled) { return; } diff --git a/app/components/emoji_picker/index.js b/app/components/emoji_picker/index.js index 1713593c1..58af58ca2 100644 --- a/app/components/emoji_picker/index.js +++ b/app/components/emoji_picker/index.js @@ -165,7 +165,6 @@ function mapStateToProps(state) { theme: getTheme(state), customEmojisEnabled: getConfig(state).EnableCustomEmoji === 'true', customEmojiPage: state.views.emoji.emojiPickerCustomPage, - serverVersion: state.entities.general.serverVersion, }; } diff --git a/app/utils/emoji_utils.js b/app/utils/emoji_utils.js index 75503da7e..d8b837cb7 100644 --- a/app/utils/emoji_utils.js +++ b/app/utils/emoji_utils.js @@ -113,3 +113,42 @@ export function getEmojiByName(emojiName) { return null; } + +// Since there is no shared logic between the web and mobile app +// this is copied from the webapp as custom sorting logic for emojis + +const defaultComparisonRule = (aName, bName) => { + return aName.localeCompare(bName); +}; + +const thumbsDownComparisonRule = (other) => + (other === 'thumbsup' || other === '+1' ? 1 : 0); +const thumbsUpComparisonRule = (other) => (other === 'thumbsdown' || other === '-1' ? -1 : 0); + +const customComparisonRules = { + thumbsdown: thumbsDownComparisonRule, + '-1': thumbsDownComparisonRule, + thumbsup: thumbsUpComparisonRule, + '+1': thumbsUpComparisonRule, +}; + +export function compareEmojis(emojiA, emojiB, searchedName) { + const aName = emojiA.name || (emojiA.aliases ? emojiA.aliases[0] : emojiA); + const bName = emojiB.name || (emojiB.aliases ? emojiB.aliases[0] : emojiB); + + // Have the emojis that contain the search appear first + const aPrefix = aName.startsWith(searchedName); + const bPrefix = bName.startsWith(searchedName); + + if (aPrefix === bPrefix) { + if (customComparisonRules[aName]) { + return customComparisonRules[aName](bName) || defaultComparisonRule(aName, bName); + } + + return defaultComparisonRule(aName, bName, searchedName); + } else if (aPrefix) { + return -1; + } + + return 1; +} \ No newline at end of file diff --git a/app/utils/emoji_utils.test.js b/app/utils/emoji_utils.test.js index a5f4b2ffe..690163352 100644 --- a/app/utils/emoji_utils.test.js +++ b/app/utils/emoji_utils.test.js @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {doesMatchNamedEmoji, hasEmojisOnly} from './emoji_utils'; +import {doesMatchNamedEmoji, hasEmojisOnly, compareEmojis} from './emoji_utils'; describe('hasEmojisOnly with named emojis', () => { const testCases = [{ @@ -283,3 +283,132 @@ describe('doesMatchNamedEmoji', () => { }); } }); + +describe('compareEmojis', () => { + test('should sort an array of emojis alphabetically', () => { + const goatEmoji = { + name: 'goat', + }; + + const dashEmoji = { + name: 'dash', + }; + + const smileEmoji = { + name: 'smile', + }; + + const emojiArray = [goatEmoji, dashEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b)); + + expect(emojiArray).toEqual([dashEmoji, goatEmoji, smileEmoji]); + }); + + test('should have partial matched emoji first', () => { + const goatEmoji = { + name: 'goat', + }; + + const dashEmoji = { + name: 'dash', + }; + + const smileEmoji = { + name: 'smile', + }; + + const emojiArray = [goatEmoji, dashEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b, 'smi')); + + expect(emojiArray).toEqual([smileEmoji, dashEmoji, goatEmoji]); + }); + + test('should be able to sort on aliases', () => { + const goatEmoji = { + aliases: [':goat:'], + }; + + const dashEmoji = { + aliases: [':dash:'], + }; + + const smileEmoji = { + aliases: [':smile:'], + }; + + const emojiArray = [goatEmoji, dashEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b)); + + expect(emojiArray).toEqual([dashEmoji, goatEmoji, smileEmoji]); + }); + + test('special case for thumbsup emoji should sort it before thumbsdown by aliases', () => { + const thumbsUpEmoji = { + aliases: ['+1'], + }; + + const thumbsDownEmoji = { + aliases: ['-1'], + }; + + const smileEmoji = { + aliases: ['smile'], + }; + + const emojiArray = [thumbsDownEmoji, thumbsUpEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b)); + + expect(emojiArray).toEqual([thumbsUpEmoji, thumbsDownEmoji, smileEmoji]); + }); + + test('special case for thumbsup emoji should sort it before thumbsdown by names', () => { + const thumbsUpEmoji = { + name: 'thumbsup', + }; + + const thumbsDownEmoji = { + name: 'thumbsdown', + }; + + const smileEmoji = { + name: 'smile', + }; + + const emojiArray = [thumbsDownEmoji, thumbsUpEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b)); + + expect(emojiArray).toEqual([smileEmoji, thumbsUpEmoji, thumbsDownEmoji]); + }); + + test('special case for thumbsup emoji should sort it when emoji is matched', () => { + const thumbsUpEmoji = { + name: 'thumbsup', + }; + + const thumbsDownEmoji = { + name: 'thumbsdown', + }; + + const smileEmoji = { + name: 'smile', + }; + + const emojiArray = [thumbsDownEmoji, thumbsUpEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b, 'thumb')); + + expect(emojiArray).toEqual([thumbsUpEmoji, thumbsDownEmoji, smileEmoji]); + }); + + test('it compares emojis when emojis are strings', () => { + const thumbsUpEmoji = 'thumbsup'; + const thumbsDownEmoji = 'thumbsdown'; + const smileEmoji = { + name: 'smile', + }; + + const emojiArray = [thumbsDownEmoji, thumbsUpEmoji, smileEmoji]; + emojiArray.sort((a, b) => compareEmojis(a, b, 'thumb')); + + expect(emojiArray).toEqual([thumbsUpEmoji, thumbsDownEmoji, smileEmoji]); + }); +});