-
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
feat: Dynamic emoji size #3561
feat: Dynamic emoji size #3561
Changes from all commits
8baee4f
7fa65be
1f1956c
379b771
156fe87
734ee3e
bcc0882
0b34771
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 |
---|---|---|
@@ -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; |
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, | ||
}; | ||
|
||
class EmojiPickerMenu extends Component { | ||
|
@@ -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), | ||
}; | ||
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. Do you need this state variable? Its not really changing anywhere in the component so I don't quite see the benefit.
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? 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. The idea of keeping it in a state opens the door for any future development for reactivity for emoji sizes. We can remove it from the state and make it a normal class property. - this.state.emojiSize
+ this.emojiSize 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.
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 👍 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 I have updated the PR |
||
} | ||
|
||
/** | ||
|
@@ -56,6 +66,7 @@ class EmojiPickerMenu extends Component { | |
<EmojiPickerMenuItem | ||
onPress={this.props.onEmojiSelected} | ||
emoji={item.code} | ||
emojiSize={this.emojiSize} | ||
/> | ||
); | ||
} | ||
|
@@ -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} /> | ||
))); |
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.
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.