Skip to content
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

feat: Dynamic emoji size #3561

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions src/pages/home/report/EmojiPickerMenu/dynamicEmojiSize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import styleVariables from '../../../../styles/variables';

const dynamicEmojiSize = (windowWidth) => {
if (windowWidth <= 320) {
return styleVariables.iconSizeExtraSmall;
}
if (windowWidth <= 480) {
return styleVariables.iconSizeNormal;
}
return styleVariables.iconSizeLarge;
};

export default dynamicEmojiSize;
6 changes: 6 additions & 0 deletions src/pages/home/report/EmojiPickerMenu/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../compo
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
import compose from '../../../../libs/compose';
import getOperatingSystem from '../../../../libs/getOperatingSystem';
import dynamicEmojiSize from './dynamicEmojiSize';

const propTypes = {
/** Function to add the selected emoji to the main compose text input */
Expand All @@ -20,6 +21,7 @@ const propTypes = {
/** The ref to the search input (may be null on small screen widths) */
forwardedRef: PropTypes.func,

/** Props related to the dimensions of the window */
...windowDimensionsPropTypes,

...withLocalizePropTypes,
Expand Down Expand Up @@ -66,6 +68,9 @@ class EmojiPickerMenu extends Component {
this.cleanupEventHandlers = this.cleanupEventHandlers.bind(this);
this.renderItem = this.renderItem.bind(this);
this.currentScrollOffset = 0;
this.emojiSize = {
fontSize: dynamicEmojiSize(this.props.windowWidth),
};

this.state = {
filteredEmojis: this.emojis,
Expand Down Expand Up @@ -302,6 +307,7 @@ class EmojiPickerMenu extends Component {
onHover={() => this.setState({highlightedIndex: index})}
emoji={code}
isHighlighted={index === this.state.highlightedIndex}
emojiSize={this.emojiSize}
/>
);
}
Expand Down
20 changes: 16 additions & 4 deletions src/pages/home/report/EmojiPickerMenu/index.native.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
import React, {Component} from 'react';
import {View, FlatList, Text} from 'react-native';
import PropTypes from 'prop-types';
import compose from '../../../../libs/compose';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../components/withWindowDimensions';
import CONST from '../../../../CONST';
import styles from '../../../../styles/styles';
import emojis from '../../../../../assets/emojis';
import EmojiPickerMenuItem from '../EmojiPickerMenuItem';
import dynamicEmojiSize from './dynamicEmojiSize';

const propTypes = {
/** Function to add the selected emoji to the main compose text input */
onEmojiSelected: PropTypes.func.isRequired,

/** Props related to the dimensions of the window */
...windowDimensionsPropTypes,
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: I see there are many places in our code where this is prop is not documented but as per our style guide it would be nice if you include a JS comment doc for this for consistency.

};

class EmojiPickerMenu extends Component {
Expand All @@ -29,6 +35,10 @@ class EmojiPickerMenu extends Component {
this.unfilteredHeaderIndices = [0, 33, 59, 87, 98, 120, 147];

this.renderItem = this.renderItem.bind(this);

this.emojiSize = {
fontSize: dynamicEmojiSize(this.props.windowWidth),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this state variable? Its not really changing anywhere in the component so I don't quite see the benefit.
How about if you just did,

emojiSize={fontSize: dynamicEmojiSize(this.props.windowWidth)}

over here.

If you think that's not too readable then we should just do this.emojiSize than save it in a state variable. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of keeping it in a state opens the door for any future development for reactivity for emoji sizes.
And adding it to the state doesn't create any overhead.

We can remove it from the state and make it a normal class property.

- this.state.emojiSize
+ this.emojiSize

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of keeping it in a state opens the door for any future development for reactivity for emoji sizes.

Making code more complex than it needs to be with the assumption that it will be modified in the future is unnecessary preoptimization. When its needed to be a state variable for any reason then its easy enough to convert so let's not have it be a state variable right now.

Sure I'm fine with class property 👍
Could you update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done I have updated the PR

}

/**
Expand Down Expand Up @@ -56,6 +66,7 @@ class EmojiPickerMenu extends Component {
<EmojiPickerMenuItem
onPress={this.props.onEmojiSelected}
emoji={item.code}
emojiSize={this.emojiSize}
/>
);
}
Expand All @@ -78,8 +89,9 @@ class EmojiPickerMenu extends Component {

EmojiPickerMenu.propTypes = propTypes;

// eslint-disable-next-line no-unused-vars
export default React.forwardRef((props, _ref) => (
export default compose(
withWindowDimensions,
)(React.forwardRef((props, ref) => (
// eslint-disable-next-line react/jsx-props-no-spreading
<EmojiPickerMenu {...props} />
));
<EmojiPickerMenu {...props} forwardedRef={ref} />
)));
19 changes: 15 additions & 4 deletions src/pages/home/report/EmojiPickerMenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@ const propTypes = {
onPress: PropTypes.func.isRequired,

/** Handles what to do when we hover over this item with our cursor */
onHover: PropTypes.func.isRequired,
onHover: PropTypes.func,

/** Whether this menu item is currently highlighted or not */
isHighlighted: PropTypes.bool.isRequired,
isHighlighted: PropTypes.bool,

/** Size of the emoji item */
emojiSize: PropTypes.shape({
fontSize: PropTypes.number,
}).isRequired,
};

const EmojiPickerMenuItem = props => (
Expand All @@ -26,19 +31,25 @@ const EmojiPickerMenuItem = props => (
pressed,
}) => ([
styles.emojiItem,
styles.pv1,
getButtonBackgroundColorStyle(getButtonState(false, pressed)),
props.isHighlighted ? styles.emojiItemHighlighted : {},
])}
>
<Hoverable onHoverIn={props.onHover}>
<Text style={styles.emojiText}>{props.emoji}</Text>
<Text style={[styles.emojiText, props.emojiSize]}>
{props.emoji}
</Text>
</Hoverable>
</Pressable>

);

EmojiPickerMenuItem.propTypes = propTypes;
EmojiPickerMenuItem.displayName = 'EmojiPickerMenuItem';
EmojiPickerMenuItem.defaultProps = {
isHighlighted: false,
onHover: () => {},
};

// Significantly speeds up re-renders of the EmojiPickerMenu's FlatList
// by only re-rendering at most two EmojiPickerMenuItems that are highlighted/un-highlighted per user action.
Expand Down
11 changes: 9 additions & 2 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -979,15 +979,22 @@ const styles = {
// Emoji Picker Styles
emojiText: {
fontFamily: fontFamily.GTA_BOLD,
fontSize: variables.iconSizeLarge,
textAlign: 'center',
...spacing.pv1,
...spacing.ph2,
},

emojiExtraSmall: {
fontSize: variables.iconSizeExtraSmall,
},

emojiLarge: {
fontSize: variables.iconSizeLarge,
},

emojiItem: {
flex: 1,
width: '12.5%',
height: 40,
textAlign: 'center',
borderRadius: 8,
},
Expand Down