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

[#3014]: Updated chat input #3259

Merged
merged 1 commit into from
Feb 16, 2018
Merged

[#3014]: Updated chat input #3259

merged 1 commit into from
Feb 16, 2018

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Feb 8, 2018

This PR includes several amazing changes:

  1. Removes react-native-emoji-picker;
  2. Restyles chat input view to make it match the new design;
  3. Adds several cool animations to input field;
  4. Refactors some parts. However, (and it's important!) it's not a refactoring PR and I would like not to make some big changes here to structure/refers/etc here.

This is how it looks:
2018-02-13 18 56 12

@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Feb 8, 2018
@alwx alwx self-assigned this Feb 8, 2018
@status-github-bot status-github-bot bot moved this from REVIEW to CONTRIBUTOR in Pipeline for QA Feb 8, 2018
@status-im status-im deleted a comment from statustestbot Feb 9, 2018
@alwx alwx force-pushed the feature/#3014 branch 2 times, most recently from 4aaf9fa to c22b80e Compare February 12, 2018 14:56
@alwx alwx changed the title WIP: [#3014]: Updated chat input [#3014]: Updated chat input Feb 13, 2018
@janherich janherich moved this from CONTRIBUTOR to REVIEW in Pipeline for QA Feb 13, 2018
Copy link
Contributor

@jeluard jeluard left a comment

Choose a reason for hiding this comment

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

Nice work!

Please make sure the new colors ns is used and
no unused requires are defined.

(ns status-im.chat.styles.input.parameter-box
(:require [status-im.ui.components.styles :as common]))

(def color-root-border "#e8ebec")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you define this in colors ns?

(def color-root-border "#e8ebec")

(def root
{:background-color common/color-white
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new colors ns.

@@ -1,7 +1,7 @@
(ns status-im.chat.styles.input.result-box
(:require [status-im.ui.components.styles :as common]))

(def color-root-border "rgba(192, 198, 202, 0.5)")
(def color-root-border "#e8ebec")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new colors ns.

@@ -0,0 +1,17 @@
(ns status-im.chat.styles.input.send-button)

(def color-send "#4360df")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new colors ns.

(def color-item-title-text "rgb(147, 155, 161)")
(def color-item-suggestion-name "rgb(98, 143, 227)")
(def color-item-border "#e8eaeb")
(def color-root-border "#e8ebec")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new colors ns.

[status-im.ui.components.react :as react]
[status-im.ui.components.icons.vector-icons :as vi]
[status-im.utils.utils :as utils]
[taoensso.timbre :as log]))
Copy link
Contributor

Choose a reason for hiding this comment

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

log not used? Maybe cleanup those requires.

(re-frame/dispatch [:send-seq-argument]))
(utils/set-timeout
(fn [] (re-frame/dispatch [:chat-input-focus :seq-input-ref]))
100))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment detailing the need for timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It was here before, and I haven't touched it. Simply moved from status-im.chat.views.input.input ns to a new one.

@alwx
Copy link
Contributor Author

alwx commented Feb 14, 2018

@jeluard done

@jeluard
Copy link
Contributor

jeluard commented Feb 14, 2018

@alwx Thanks! For colors we know favor color name (and comment usage). Goal is to make sure we don't have too many similar colors duplicated.

Could you check with design team those colors are supposed to be different from existing grays? (already 4 or 5)

@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24">
<path fill="#6E777E" fill-rule="nonzero" d="M7 5a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2V7a2 2 0 0 0-2-2H7zm0-2h10a4 4 0 0 1 4 4v10a4 4 0 0 1-4 4H7a4 4 0 0 1-4-4V7a4 4 0 0 1 4-4zm6.618 5H14a.618.618 0 0 1 .553.894l-3.277 6.553a1 1 0 0 1-.894.553H10a.618.618 0 0 1-.553-.894l3.277-6.553A1 1 0 0 1 13.618 8z"/>
Copy link
Member

Choose a reason for hiding this comment

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

better to clean fill, to do not confuse, because this value not used

@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" width="22" height="22" viewBox="0 0 22 22">
<path fill="#FFF" fill-rule="evenodd" d="M11.882 8.686l2.047 2.057a.87.87 0 1 0 1.227-1.233l-3.53-3.547a.865.865 0 0 0-.537-.251.866.866 0 0 0-.718.251L6.841 9.51a.88.88 0 0 0 0 1.234.864.864 0 0 0 1.228-.001l2.049-2.059v6.732a.88.88 0 0 0 .882.878c.49 0 .882-.393.882-.878v-6.73z"/>
Copy link
Member

Choose a reason for hiding this comment

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

better to clean fill, to do not confuse, because this value not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not true. seems like the opposite thing is true — color property for svgs doesn't work

@@ -215,7 +215,6 @@

:sharing-share "Deel..."
:type-a-message "Tik ‘n boodskap..."
:type-a-command "Begin ‘n opdrag tik..."
Copy link
Member

Choose a reason for hiding this comment

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

we decided and agreed to do not delete translations,

@jeluard jeluard moved this from REVIEW to TO TEST in Pipeline for QA Feb 15, 2018
@alwx alwx removed the request for review from rasom February 15, 2018 13:33
@annadanchenko annadanchenko moved this from TO TEST to IN TESTING in Pipeline for QA Feb 15, 2018
@annadanchenko annadanchenko self-assigned this Feb 15, 2018
@alwx alwx force-pushed the feature/#3014 branch 2 times, most recently from 4cd3f7f to 6177009 Compare February 15, 2018 14:36
@annadanchenko
Copy link

on PR build 24:

Issue 1: No contacts are shown in suggestions for send/request in Group chat. Both iOS and Android
Steps:

Issue 2 (Android only)
Suggestions title (Send transaction) is overlapped by chat header and is not fully visible. Google Pixel XL.
screenshot_20180215-163847

note that on iOS it's above chat header and can be read ok.
img_2613

@alwx
Copy link
Contributor Author

alwx commented Feb 16, 2018

@annadanchenko can be re-tested again

@annadanchenko
Copy link

annadanchenko commented Feb 16, 2018

Issues 1-2 are fixed.
2 new are found:

Issue 3: tap on Return key on device keyboard when password is entered in Console shows error. Expected: value is sent to Console (same as if tap on "send" button in the input field". Note: this is the only place where Enter behaves like this, in other places it should add new line in the input field
TF: https://app.testfairy.com/projects/4803622-status/builds/7716052/sessions/14/?accessToken=vrhBUlcsxxH5h0rQRxIcsV8qUoU

00:12 I/ReactNativeJS: 
00:12 I/ReactNativeJS: missing_protocol@index.android.bundle:12:114606
00:12 I/ReactNativeJS: _deref@index.android.bundle:12:156439
00:12 I/ReactNativeJS: deref@index.android.bundle:12:214082
00:12 I/ReactNativeJS: index.android.bundle:12:8035926
00:12 I/ReactNativeJS: index.android.bundle:12:8036087
00:12 I/ReactNativeJS: C@index.android.bundle:49:1462
00:12 I/ReactNativeJS: invokeGuardedCallback@index.android.bundle:49:725
00:12 I/ReactNativeJS: invokeGuardedCallbackAndCatchFirstError@index.android.bundle:49:840
00:12 I/ReactNativeJS: k@index.android.bundle:49:1764
00:12 I/ReactNativeJS: executeDispatchesInOrder@index.android.bundle:49:2521
00:12 I/ReactNativeJS: jo@index.android.bundle:49:62490
00:12 I/ReactNativeJS: Oo@index.android.bundle:49:62597
00:12 I/ReactNativeJS: Uo@index.android.bundle:49:62428
00:12 I/ReactNativeJS: processEventQueue@index.android.bundle:49:63577
00:12 I/ReactNativeJS: handleTopLevel@index.android.bundle:49:68294
00:12 I/ReactNativeJS: index.android.bundle:49:68534
00:12 I/ReactNativeJS: batchedUpdates@index.android.bundle:49:93160
00:12 I/ReactNativeJS: A@index.android.bundle:49:3891
00:12 I/ReactNativeJS: batchedUpdates@index.android.bundle:49:3978
00:12 I/ReactNativeJS: _receiveRootNodeIDEvent@index.android.bundle:49:68505
00:12 I/ReactNativeJS: receiveEvent@index.android.bundle:49:68609
00:12 I/ReactNativeJS: value@index.android.bundle:27:3177
00:12 I/ReactNativeJS: index.android.bundle:27:911
00:12 I/ReactNativeJS: value@index.android.bundle:27:2606
00:12 I/ReactNativeJS: value@index.android.bundle:27:883
00:12 I/ReactNativeJS: [native code]
00:12 I/ReactNativeJS:  

Issue 4: Second "/send" command is shown in commands list if chat contains 1 or more Request messages sent to this contact. Expected: Only one /send command is shown in the suggestions list
Steps:

  • in group or in 1:1 chat send 2 requests to contact B
  • as contact B: open the chat with 2 request messages and tap on "command" icon to open commands suggestions list. Check how many times "/send" command is shown.

img_2614

Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants