Skip to content

Fix emoji offset issues #8902

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.

Sign up for GitHub

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

Merged
chiragsalian merged 12 commits into from
Nov 14, 2022
Merged

Fix emoji offset issues #8902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9c35a84
Fix emoji offset issues
sobitneupane May 9, 2022
0d9d205
Fix the height of emoji
sobitneupane May 27, 2022
8ea2fff
Merge branch 'main' into sn_bug-emoji-positioning
sobitneupane Jun 16, 2022
eb49313
Merge main resolving conflict in EmojiSkinToneList
sobitneupane Sep 5, 2022
98f45b7
Merge branch 'main' into sn_bug-emoji-positioning
sobitneupane Sep 29, 2022
3c697bc
Change intitalNumToRender for better performance
sobitneupane Sep 30, 2022
37622b3
Remove initalNumToRender for better performance
sobitneupane Oct 20, 2022
1dd885d
Merge branch 'main' into sn_bug-emoji-positioning
sobitneupane Oct 20, 2022
7a226a6
Change emoji height
sobitneupane Nov 2, 2022
5ca7584
Merge branch 'main' into sn_bug-emoji-positioning
sobitneupane Nov 8, 2022
18d826f
Align emoji center in IOS
sobitneupane Nov 9, 2022
450146f
Fix lint error
sobitneupane Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,8 @@ const CONST = {
WIDTH: 320,
HEIGHT: 400,
},
NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 300,
EMOJI_PICKER_ITEM_HEIGHT: 40,
NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 298,
EMOJI_PICKER_ITEM_HEIGHT: 32,
EMOJI_PICKER_HEADER_HEIGHT: 38,
COMPOSER_MAX_HEIGHT: 125,
CHAT_FOOTER_MIN_HEIGHT: 65,
Expand Down
4 changes: 2 additions & 2 deletions src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,8 @@ class EmojiPickerMenu extends Component {
let targetOffset = this.currentScrollOffset;
if (offsetAtEmojiBottom - this.currentScrollOffset >= CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT) {
targetOffset = offsetAtEmojiBottom - CONST.NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT;
} else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_ITEM_HEIGHT <= this.currentScrollOffset) {
targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_ITEM_HEIGHT;
} else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT <= this.currentScrollOffset) {
targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT;
Comment on lines +348 to +349
Copy link
Copy Markdown
Member

@parasharrajat parasharrajat May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain this change?

Copy link
Copy Markdown
Contributor Author

@sobitneupane sobitneupane May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Emoji Picker Header is what remains at the top, we need to subtract emoji picker header height not emoji height. Here, this calculation is trying to detect if emoji has gone out of view while pressing up arrow.

Copy link
Copy Markdown
Member

@parasharrajat parasharrajat Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's put this as a comment in the code.

Copy link
Copy Markdown
Contributor Author

@sobitneupane sobitneupane Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat Any suggestion?

        } else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT <= this.currentScrollOffset) {
+            // As Emoji Picker Header is what remains at the top, EMOJI_PICKER_HEADER_HEIGHT is subtracted from offsetAtEmojiTop
            targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT;
        }

}
if (targetOffset !== this.currentScrollOffset) {
// Disable pointer events so that onHover doesn't get triggered when the items move while we're scrolling
Expand Down
1 change: 0 additions & 1 deletion src/components/EmojiPicker/EmojiPickerMenuItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ const EmojiPickerMenuItem = props => (
style={({
pressed,
}) => ([
styles.pv1,
StyleUtils.getButtonBackgroundColorStyle(getButtonState(false, pressed)),
props.isHighlighted ? styles.emojiItemHighlighted : {},
styles.emojiItem,
Expand Down
9 changes: 5 additions & 4 deletions src/components/EmojiPicker/EmojiSkinToneList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,17 +59,18 @@ class EmojiSkinToneList extends Component {
() => this.setState(prev => ({isSkinToneListVisible: !prev.isSkinToneListVisible}))
}
style={[
styles.pv1,
Copy link
Copy Markdown
Member

@parasharrajat parasharrajat Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this back to the emoji but include this on the emoji height so that we can cover this value. Motive should always be to keep the same UI. I see that UI is little bit different from staging.

Copy link
Copy Markdown
Contributor Author

@sobitneupane sobitneupane Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the reason for slight change in UI. It is due to the introduction of EMOJI_PICKER_ITEM_HEIGHT and variables.emojiLineHeight.

styles.flex1,
styles.flexRow,
styles.alignSelfCenter,
styles.justifyContentStart,
styles.alignItemsCenter,
]}
>
<Text style={[styles.emojiText, styles.ph2, styles.emojiItem, styles.textNoWrap]}>
{selectedEmoji.code}
</Text>
<View style={[styles.emojiItem, styles.justifyContentCenter]}>
<Text style={[styles.emojiText, styles.ph2, styles.textNoWrap]}>
{selectedEmoji.code}
</Text>
</View>
<Text style={[styles.emojiSkinToneTitle]}>
{this.props.translate('emojiPicker.skinTonePickerLabel')}
</Text>
Expand Down
4 changes: 3 additions & 1 deletion src/libs/EmojiUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ function getDynamicHeaderIndices(emojis) {
function getDynamicSpacing(emojiCount, suffix) {
const spacerEmojis = [];
let modLength = CONST.EMOJI_NUM_PER_ROW - (emojiCount % CONST.EMOJI_NUM_PER_ROW);
while (modLength > 0) {

// Empty spaces is pushed if the given row has less than eight emojis
while (modLength > 0 && modLength < CONST.EMOJI_NUM_PER_ROW) {
spacerEmojis.push({
code: `${CONST.EMOJI_SPACER}_${suffix}_${modLength}`,
spacer: true,
Expand Down
5 changes: 5 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import optionAlternateTextPlatformStyles from './optionAlternateTextPlatformStyl
import pointerEventsNone from './pointerEventsNone';
import pointerEventsAuto from './pointerEventsAuto';
import overflowXHidden from './overflowXHidden';
import CONST from '../CONST';

const picker = {
backgroundColor: 'transparent',
Expand Down Expand Up @@ -1475,12 +1476,16 @@ const styles = {
fontSize: variables.emojiSize,
...spacing.pv0,
...spacing.ph0,
lineHeight: variables.emojiLineHeight,
},

emojiItem: {
width: '12.5%',
textAlign: 'center',
borderRadius: 8,
paddingTop: 2,
paddingBottom: 2,
height: CONST.EMOJI_PICKER_ITEM_HEIGHT,
},

emojiItemHighlighted: {
Expand Down
1 change: 1 addition & 0 deletions src/styles/variables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default {
iconSizeExtraLarge: 40,
iconSizeSuperLarge: 60,
emojiSize: 20,
emojiLineHeight: 28,
iouAmountTextSize: 40,
mobileResponsiveWidthBreakpoint: 800,
tabletResponsiveWidthBreakpoint: 1024,
Expand Down