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

Make ReactionPicker full screen with search by emoji title #780

Closed
wants to merge 14 commits into from

Conversation

IlarionHalushka
Copy link
Contributor

@RocketChat/ReactNative
Closes #765

Hi @diegolmello ! This is my first PR that fixes the issue "ReactionsPicker should be full screen" including search by emoji title.

Screenshot IOS: http://prntscr.com/n72mjg
Screenshot Android: http://prntscr.com/n73fbg
Video: https://drive.google.com/file/d/1UY7I5eWg5X60i_nCdQtjsnJ2FlTqqQKO/view?usp=sharing

  • Tested on both IOS and Android.
  • Tested opening ReactionsPicker via ActionPicker (on message long press) and with plus button.
  • Tested that EmojiPicker is not broken with the changes made.

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2019

CLA assistant check
All committers have signed the CLA.

app/containers/EmojiPicker/EmojiCategory.js Show resolved Hide resolved
app/containers/EmojiPicker/EmojiCategory.js Show resolved Hide resolved
app/containers/EmojiPicker/ReactionPicker.js Show resolved Hide resolved
app/containers/EmojiPicker/ReactionPicker.js Outdated Show resolved Hide resolved
app/containers/EmojiPicker/ReactionPicker.js Outdated Show resolved Hide resolved
app/views/ReactionPickerView/index.js Outdated Show resolved Hide resolved
app/containers/EmojiPicker/styles.js Outdated Show resolved Hide resolved
app/containers/EmojiPicker/styles.js Show resolved Hide resolved
app/views/RoomView/index.js Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (develop@7a80550). Click here to learn what that means.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #780   +/-   ##
==========================================
  Coverage           ?   86.92%           
==========================================
  Files              ?       39           
  Lines              ?      673           
  Branches           ?      117           
==========================================
  Hits               ?      585           
  Misses             ?       71           
  Partials           ?       17
Impacted Files Coverage Δ
app/i18n/locales/ru.js 100% <ø> (ø)
app/i18n/locales/zh-CN.js 100% <ø> (ø)
app/i18n/locales/fr.js 100% <ø> (ø)
app/i18n/locales/en.js 100% <ø> (ø)
app/i18n/locales/pt-BR.js 100% <ø> (ø)
app/i18n/locales/de.js 100% <ø> (ø)
app/i18n/locales/pt-PT.js 100% <ø> (ø)
app/containers/message/Message.js 90.9% <91.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a80550...426081c. Read the comment docs.

@diegolmello
Copy link
Member

@IlarionHalushka Can you resolve conflicts?

@IlarionHalushka
Copy link
Contributor Author

@diegolmello I resolved conflicts and

Made a few performance improvements like: using caching of images (custom emoji), use debounce on handleSearch, remove unneeded View wrappers.

Then I decided to hide frequently used if there are 0 items.

Finally fixed header title, not sure why it was not displaying when I used static navigationOptions inside the ReactionView. Maybe it's because the screen is being opened using ActionSheet...

# Conflicts:
#	__tests__/__snapshots__/Storyshots.test.js.snap
#	app/containers/EmojiPicker/CustomEmoji.js
#	app/containers/message/Message.js
#	app/containers/message/index.js
#	app/index.js
#	app/views/RoomView/index.js
#	package.json
#	yarn.lock
@@ -1,12 +1,12 @@
import React from 'react';
import { Image } from 'react-native';
import { ViewPropTypes, Image } from 'react-native';
Copy link
Member

Choose a reason for hiding this comment

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

Don't use ViewPropTypes

@@ -33,7 +38,7 @@ export default class EmojiCategory extends React.Component {
onEmojiSelected: PropTypes.func,
emojisPerRow: PropTypes.number,
width: PropTypes.number
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Why?

initialNumToRender={45}
getItemLayout={(data, index) => ({ length: this.size, offset: this.size * index, index })}
initialNumToRender={30}
getItemLayout={(data, index) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Create a function

@@ -1,44 +1,56 @@
const list = ['frequentlyUsed', 'custom', 'people', 'nature', 'food', 'activity', 'travel', 'objects', 'symbols', 'flags'];
const titles = ['FREQUENTLY USED', 'CUSTOM', 'PEOPLE', 'NATURE', 'FOOD', 'ACTIVITY', 'TRAVEL', 'OBJECTS', 'SYMBOLS', 'FLAGS'];
Copy link
Member

Choose a reason for hiding this comment

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

i18n

@@ -94,12 +97,12 @@ export default class EmojiPicker extends Component {
database.write(() => {
database.create('frequentlyUsedEmoji', emoji, true);
});
})
});
Copy link
Member

Choose a reason for hiding this comment

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

Why?

OnboardingView: {
screen: OnboardingView,
header: null
const OutsideStack = createStackNavigator(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@@ -110,6 +121,12 @@ const ChatsStack = createStackNavigator({
SearchMessagesView,
SelectedUsersView,
ThreadMessagesView,
ReactionPickerView: {
screen: ReactionPickerView,
navigationOptions: {
Copy link
Member

Choose a reason for hiding this comment

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

Do it on the ReactionPickerView

}, {
defaultNavigationOptions: defaultHeader
});
const NewMessageStack = createStackNavigator(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

initialRouteName: 'AuthLoading'
}
));
const App = createAppContainer(
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@@ -17,7 +16,15 @@ export default function messages(state = initialState, action) {
return {
...state,
showActions: true,
actionMessage: action.actionMessage
actionMessage: {
Copy link
Member

Choose a reason for hiding this comment

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

This is error prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReactionsPicker should be full screen
4 participants