Fix #3013 Swipe chat based deletion #3192
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
|
Thanks for making your first PR here! |
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
2 times, most recently
from
3934ec8
to
18bf0a9
Compare
flexsurfer
requested changes
Feb 2, 2018
| @@ -36,7 +36,7 @@ | |||
| (.stopAnimation anim-value)) | |||
|
|
|||
| (defn value [anim-value] | |||
| (.-value anim-value)) | |||
| (.-_value anim-value)) | |||
There was a problem hiding this comment.
could you please elaborate on this change?
There was a problem hiding this comment.
Previous method does not work, check out stackoverflow here
There was a problem hiding this comment.
yeah it's not idiomatic to read value we should try to avoid this
There was a problem hiding this comment.
@flexsurfer This function is used nowhere, do you suggest delete this function? Otherwise the previous function will make no sense since it does not work as expected.
| @@ -164,3 +166,20 @@ | |||
| :width 14 | |||
| :height 9 | |||
| :tint-color :white}) | |||
|
|
|||
| (def delete-icon | |||
| {:color component.styles/color-red}) | |||
There was a problem hiding this comment.
could you use colors/red here
| (def delete-icon | ||
| {:color component.styles/color-red}) | ||
|
|
||
| (def delete-icon-highlight |
There was a problem hiding this comment.
map fields indentation (code style)
| :width 800 | ||
| :background-color component.styles/color-red-3}) | ||
|
|
||
| (def delete-icon-container |
There was a problem hiding this comment.
map fields indentation (code style)
| @@ -15,6 +15,9 @@ | |||
| [status-im.utils.gfycat.core :as gfycat] | |||
| [status-im.constants :as const] | |||
| [taoensso.timbre :as log] | |||
| [status-im.ui.components.animation :as anim] | |||
There was a problem hiding this comment.
please use full names :as animation , :as responder
| offset-x (anim/create-value 0) | ||
| swipe-pan-responder (resp/swipe-pan-responder offset-x styles/delete-button-width) | ||
| swipe-pan-handler (resp/pan-handlers swipe-pan-responder)] | ||
| [react/view (merge {:style {}} swipe-pan-handler) |
There was a problem hiding this comment.
what the purpose of this merge?
There was a problem hiding this comment.
This is an mistake, deleted 😀
| @@ -0,0 +1,33 @@ | |||
| (ns status-im.ui.screens.home.animations.responder | |||
| (:require [status-im.ui.components.react :refer [animated pan-responder]] | |||
There was a problem hiding this comment.
please use :as instead :refer
There was a problem hiding this comment.
do you mean using :as across the project instead of :refer? why ? This animated is a native class in react native. But I will remove it, because it is not used in the file 😅.
There was a problem hiding this comment.
yes, using :as across the project instead of :refer
There was a problem hiding this comment.
animation comment for next line, instead :as anim
|
it would be great to have component, something like [deletable {:on-delete #()}
[any-hiccup]]@hanwencheng is it possible? |
|
Great work @hanwencheng ! Please look at comments added by @flexsurfer |
|
@flexsurfer Thanks for reviewing! The prototype you mentioned is React Higher-Order Component, I frankly do not know the implementation of it in re-frame. |
|
@hanwencheng |
|
Updated, please review again @flexsurfer . |
jeluard
suggested changes
Feb 5, 2018
| (- base-value (.-dx gesture)))) | ||
|
|
||
| (defn on-start [_ gesture] | ||
| (> (Math/abs (.-dx gesture)) 10)) |
There was a problem hiding this comment.
Wouldn't js/Math.abs be more idiomatic?
There was a problem hiding this comment.
In this project, there is two place use Math/abs, and no occurrence of js/math.abs yet.
There was a problem hiding this comment.
Those are probably outdated: proper ClojureScript interop syntax for external calls is to use js/
| :bottom 0 | ||
| :right -800 | ||
| :width 800 | ||
| :background-color component.styles/color-red-3}) |
There was a problem hiding this comment.
This is a background color which show in issue #3013 (as screenshot), which is light red.
There was a problem hiding this comment.
Please introduce this color in colors ns as light-red
| (def delete-icon-container | ||
| {:flex 1 | ||
| :width delete-button-width | ||
| :justify-content :center |
| [react/view styles/chat-icon-container | ||
| [chat-icon.screen/chat-icon-view-chat-list chat-id group-chat name color online]] | ||
| [react/view styles/chat-info-container | ||
| [react/view styles/item-upper-container |
There was a problem hiding this comment.
Can we make this whole block more modular by extracting some function?
There was a problem hiding this comment.
@jeluard, thanks for reviewing. A idea to make it modular is to have a structure of:
home-list-item -> deletable wrapper -> chat-item/ browser-item.
Which is discussed with @flexsurfer in previous comment.
This part in the I don't think need to be changed, because currently chat-item is different from browser-item, there is rarely common part except of this deletable wrapper. This structure now is good for comparison and further refactoring.
There was a problem hiding this comment.
My comment was not about having reusable parts but splitting it in smaller chunks to improve readability.
Looking at it in more details this is probably good enough.
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
2 times, most recently
from
8d8cd17
to
d3e788e
Compare
|
@hanwencheng please rename the file it should be |
flexsurfer
reviewed
Feb 7, 2018
resources/icons/delete-red.svg
Outdated
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> | |||
| <path fill="#FF2D55" d="M7.00001034,5 C7.004297,3.34182361 8.34093668,2 10,2 L14,2 C15.6555111,2 16.9973018,3.34290405 16.9999959,5 L18.5454545,5 C19.9047426,5 21,6.12948418 21,7.5 C21,8.87051582 19.9047426,10 18.5454545,10 L5.45454545,10 C4.09525738,10 3,8.87051582 3,7.5 C3,6.12948418 4.09525738,5 5.45454545,5 L7.0000096,5 Z M14.9999879,5 C14.9973085,4.44655807 14.5500233,4 14,4 L10,4 C9.44811686,4 9.00424986,4.44383154 9.00003034,5 L14.9999888,5 Z M12,13.5857864 L13.2928932,12.2928932 C13.6834175,11.9023689 14.3165825,11.9023689 14.7071068,12.2928932 C15.0976311,12.6834175 15.0976311,13.3165825 14.7071068,13.7071068 L13.4142136,15 L14.7071068,16.2928932 C15.0976311,16.6834175 15.0976311,17.3165825 14.7071068,17.7071068 C14.3165825,18.0976311 13.6834175,18.0976311 13.2928932,17.7071068 L12,16.4142136 L10.7071068,17.7071068 C10.3165825,18.0976311 9.68341751,18.0976311 9.29289322,17.7071068 C8.90236893,17.3165825 8.90236893,16.6834175 9.29289322,16.2928932 L10.5857864,15 L9.29289322,13.7071068 C8.90236893,13.3165825 8.90236893,12.6834175 9.29289322,12.2928932 C9.68341751,11.9023689 10.3165825,11.9023689 10.7071068,12.2928932 L12,13.5857864 Z M17,12 C17,11.4477153 17.4477153,11 18,11 C18.5522847,11 19,11.4477153 19,12 L19,19.998 C19,21.604 17.953,23 16.4985,23 L7.5,23 C6.047,23 5,21.604 5,20 L5,12 C5,11.4477153 5.44771525,11 6,11 C6.55228475,11 7,11.4477153 7,12 L7,20 C7,20.604 7.297,21 7.5,21 L16.4985,21 C16.703,21 17,20.604 17,19.998 L17,12 Z M18.5454545,8 C18.7861665,8 19,7.77948418 19,7.5 C19,7.22051582 18.7861665,7 18.5454545,7 L5.45454545,7 C5.21383352,7 5,7.22051582 5,7.5 C5,7.77948418 5.21383352,8 5.45454545,8 L18.5454545,8 Z"/> | |||
flexsurfer
reviewed
Feb 7, 2018
| [react/touchable-highlight {:style styles/delete-icon-highlight | ||
| :on-press #(re-frame/dispatch [:remove-chat chat-id])} | ||
| [react/view {:style styles/delete-icon-container} | ||
| [vector-icons/icon :icons/delete-red styles/delete-icon]]]]]))) |
There was a problem hiding this comment.
[vector-icons/icon :icons/delete {:style styles/delete-icon :color colors/red}]]
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
from
d3e788e
to
59332c1
Compare
flexsurfer
approved these changes
Feb 7, 2018
|
@hanwencheng Looks like you forgot to update the icon keyword (still |
jeluard
reviewed
Feb 8, 2018
| [react/touchable-highlight {:style styles/delete-icon-highlight | ||
| :on-press #(re-frame/dispatch [:remove-chat chat-id])} | ||
| [react/view {:style styles/delete-icon-container} | ||
| [vector-icons/icon :icons/delete-red {:color colors/red}]]]]]))) |
There was a problem hiding this comment.
@hanwencheng please take a look
There was a problem hiding this comment.
@jeluard , thanks for pointing that, my mistake, updated now
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
from
59332c1
to
dc21de4
Compare
jeluard
approved these changes
Feb 8, 2018
|
Branch: PR-3192 |
|
Hey @hanwencheng thanks for a contribution! And one more thing: when sometimes I add a new person from contacts it will show on a chats screen as ready for deletion. Screencast is here https://www.dropbox.com/s/of59aqsjfv7kyv0/ScreenRecording_02-08-2018%2012-11-13.MP4?dl=0
|
Automated test results:test_qr_code_and_its_value:white_check_mark::Test Steps & Error message:test_browse_link_entering_url_in_dapp_view:white_check_mark::Test Steps & Error message:test_network_switch:white_check_mark::Test Steps & Error message:test_transaction_send_command_wrong_password:white_check_mark::Test Steps & Error message:test_send_transaction_from_daap:white_check_mark::Test Steps & Error message:test_contact_profile_view:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_later:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_now:white_check_mark::Test Steps & Error message:test_send_stt_from_wallet_via_enter_contact_code:white_check_mark::Test Steps & Error message:test_transaction_send_command_group_chat:white_check_mark::Test Steps & Error message:test_send_eth_to_request_in_group_chat:white_check_mark::Test Steps & Error message:test_send_eth_to_request_in_one_to_one_chat:white_check_mark::Test Steps & Error message:test_send_eth_to_request_from_wallet:x:Test Steps & Error message:
|
|
@hanwencheng please, take a look at |
|
@antdanchenko thanks, will check it today. |
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
2 times, most recently
from
44dfcb2
to
e67873f
Compare
Automated test results:test_qr_code_and_its_value:white_check_mark::Test Steps & Error message:test_contact_profile_view:x:Test Steps & Error message:
test_browse_link_entering_url_in_dapp_view:white_check_mark::Test Steps & Error message:test_network_switch:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_now:white_check_mark::Test Steps & Error message:test_send_eth_to_request_in_one_to_one_chat:x:Test Steps & Error message:
test_transaction_send_command_wrong_password:x:Test Steps & Error message:
test_send_transaction_from_daap:white_check_mark::Test Steps & Error message:test_transaction_send_command_group_chat:white_check_mark::Test Steps & Error message:test_public_chat:white_check_mark::Test Steps & Error message:test_send_stt_from_wallet_via_enter_contact_code:white_check_mark::Test Steps & Error message:test_one_to_one_chat_messages_and_delete_chat:white_check_mark::Test Steps & Error message:test_send_eth_from_wallet_sign_later:white_check_mark::Test Steps & Error message:test_transaction_send_command_one_to_one_chat:x:Test Steps & Error message:
test_send_eth_to_request_in_group_chat:white_check_mark::Test Steps & Error message:test_group_chat_messages:white_check_mark::Test Steps & Error message:test_send_eth_to_request_from_wallet:white_check_mark::Test Steps & Error message: |
|
Branch: PR-3192 Tested on both Android and iOS - no issues found |
|
@hanwencheng Please squash your commits |
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
3 times, most recently
from
03de947
to
2679736
Compare
|
Build fails because of the following: |
hanwencheng
force-pushed
the
feature/swipe-chat-delete
branch
from
2679736
to
84e27c2
Compare
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
flexsurfer
force-pushed
the
feature/swipe-chat-delete
branch
from
84e27c2
to
f4f1ab4
Compare
|
@hanwencheng I had to revert this PR because it introduces a bug in develop, when you create a new account on a fresh install the chat-list in home is empty. Can you have a look and fix that please ? |
|
@hanwencheng it's empty for old accounts too |
|
I pulled the develop branch, and now build is keep failing, will check it soon. |
|
@hanwencheng How can we help? |
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.
Swipe chat based deletion of chat
If you submit PR for issue with bounty then write here Fixes #NN where NN is issue number
fixes #3013
Summary:
Add pan-responder for inner panel.
Review notes (optional):
anim/valuefunction is incorrect, so I correct it.touchable-highlightfor navigation need to be wrapped inside pan-responoder because:home-list-browser-item-inner-viewis introduced from previous commit 7e78acc @flexsurfer , now there is onlydelete-chathandler. It need to be decided how to delete browser item, and then make an animation-container for bothhome-list-chat-item-inner-viewandhome-list-browser-item-inner-view, which are out of the topic of this issue.Steps to test:
status: ready