[#13517] UI Component - Message Input #13620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (60)
|
flexsurfer
force-pushed
the
feature/message-input
branch
2 times, most recently
from
8584306
to
ffc4798
Compare
flexsurfer
force-pushed
the
feature/message-input
branch
2 times, most recently
from
6465278
to
c442cea
Compare
flexsurfer
force-pushed
the
feature/message-input
branch
from
a377ad5
to
3197167
Compare
flexsurfer
force-pushed
the
feature/message-input
branch
from
3197167
to
a0d3f0d
Compare
flexsurfer
changed the title
Parveshdhull
approved these changes
Sep 3, 2022
There was a problem hiding this comment.
Hi @flexsurfer, Great Work. PR looks good to me.
Leaving a few small questions and comments.
| @@ -19,7 +20,12 @@ | |||
| [quo.components.list.item :as list-item] | |||
| [status-im.ui.screens.chat.photos :as photos] | |||
| [reagent.core :as reagent] | |||
| [clojure.string :as string])) | |||
| [clojure.string :as string] | |||
| [quo2.components.button :as quo2] | |||
There was a problem hiding this comment.
if we import another quo2 component then we have to use it as quo2.text etc. So, should we use quo2.button here, for naming consistency?
Comment on lines
145
to
148
| (defn show-send [{:keys [actions-ref send-ref sticker-ref]}] | ||
| (quo.react/set-native-props actions-ref #js {:width 0 :left -88}) | ||
| (quo.react/set-native-props send-ref #js {:width nil :right nil}) | ||
| (quo.react/set-native-props sticker-ref #js {:width 0 :right -100})) | ||
| ;(quo.react/set-native-props actions-ref #js {:width 0 :left -88}) | ||
| (quo.react/set-native-props send-ref #js {:width nil :right nil})) | ||
| ;(quo.react/set-native-props sticker-ref #js {:width 0 :right -100})) |
There was a problem hiding this comment.
wouldn't these changes will affect old ui? or these are just temporary only for testing
| :state :min ;:min, :custom-chat-available, :custom-chat-unavailable, :max | ||
| :clear false}) | ||
| keyboard-was-shown (atom false)] | ||
| (fn [] |
There was a problem hiding this comment.
hm good question, i would say no, but I'm not sure :)
Comment on lines
+56
to
+58
| (merge quo2.typography/font-regular | ||
| quo2.typography/paragraph-1 | ||
| {:flex 1 |
There was a problem hiding this comment.
should we use quo2.text instead? It will create same results now, but quo2.text will evolve to fix letter-spacing and line-height issue, which might not be covered by just using these params.
Comment on lines
133
to
137
| :chat.ui/clear-inputs | ||
| (fn [] | ||
| (reset! input-texts {}) | ||
| (reset! mentions-enabled {}) | ||
| (reset! chat-input-key 1))) |
There was a problem hiding this comment.
its weird, lint is failing locally. But tests are showing as passed in pr. And on opening details, it is showing result of first build, not current one.
There was a problem hiding this comment.
wow, now as I approved the PR, it also giving me option to merge.
Maybe @jakubgs , can give it a look
There was a problem hiding this comment.
Good point. That's because we pipe output into tee, but we don't use pipefail option. I'll fix it tomorrow.
There was a problem hiding this comment.
There you go:
flexsurfer
force-pushed
the
feature/message-input
branch
from
a0d3f0d
to
efc8548
Compare
100% of end-end tests have passedPassed tests (87)Click to expandClass TestWalletManagementDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestSendTxDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestRestoreOneDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestPairingSyncMultipleDevicesMerged:
|
|
@flexsurfer thanx for the PR. No regression found related to message input field. All e2e have passed. Ready for merge. |
flexsurfer
force-pushed
the
feature/message-input
branch
from
efc8548
to
c7049dd
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #13517
QA: check if old message input works fine