Allow address copying in the QR code viewer, wallet & contact screens #3957

go1t · 2018-04-22T02:13:56Z

Summary:

Allows long tap to copy the wallet address from the qr code viewer screen. This is done by adding selectable to the text.

The highlight box will also show when the long press is performed, and go away when anywhere else is touched

status: ready

ghost · 2018-04-22T02:13:59Z

Hey @go1t, thanks for making your first pull request in status-react! ❤️

pablanopete · 2018-04-23T16:54:38Z

@go1t Thanks for joining! Would love to learn more about your journey into Status Open bounty, mind sending me a DM when you get a chance on https://chat.status.im ! I'm Hutch on there

go1t · 2018-04-23T22:47:50Z

@pablanopete DM'ed!

flexsurfer · 2018-04-26T06:37:15Z

thanks for contribution @go1t , have you seen this #3646 (comment) ?

go1t · 2018-04-26T21:27:25Z

@flexsurfer yep I have seen that. Unfortunately component cant do the highlight that would persist after long tap is released, so I had to use a bit of a state here to get that highlight to show up. But yeah it should be working

jeluard · 2018-04-27T13:15:14Z

src/status_im/ui/components/highlight.cljs

+
+(defn highlight-provider [render-fn]
+  (let [highlighting (reagent/atom false)]
+    [react/touchable-without-feedback {:on-press   #(reset! highlighting false)}


Extra space?

jeluard · 2018-04-27T13:15:37Z

src/status_im/ui/components/highlight.cljs

+(defn highlight-provider [render-fn]
+  (let [highlighting (reagent/atom false)]
+    [react/touchable-without-feedback {:on-press   #(reset! highlighting false)}
+     [react/view {:flex 1}


We have components/flex for this.

I can't find it :( I only saw this code being used elsewhere

Here it is: https://github.com/status-im/status-react/blob/develop/src/status_im/ui/components/styles.cljs#L58

jeluard · 2018-04-27T13:16:29Z

src/status_im/ui/screens/profile/contact/styles.cljs

@@ -55,3 +56,5 @@
  {:padding-top      26
   :background-color colors/white})

+(def address-highlight
+  {:background-color styles/color-light-blue-transparent})


Please use colors ns.

jeluard · 2018-04-27T13:17:04Z

src/status_im/ui/screens/profile/contact/views.cljs

+                  :accessibility-label   accessibility-label
+                  :selectable            true
+                  :suppress-highlighting true
+                  :on-long-press         #(reset! highlight-address true)}


highlight-address? maybe?

sorry why do we need the question mark here?

To better reflect that this is a boolean value?

oh I wasn't aware that it's a naming convention. Fixed

jeluard · 2018-04-27T13:19:08Z

src/status_im/ui/components/qr_code_viewer/styles.cljs

   :text-align        :center
   :font-size         15
   :letter-spacing    -0.2
   :line-height       20})

 (def done-button-text
  {:color colors/white})
+
+(def address-highlight
+  {:background-color styles/color-light-blue-transparent})


Please use colors ns

jeluard · 2018-04-27T13:20:56Z

src/status_im/ui/screens/profile/user/views.cljs

@@ -68,8 +69,9 @@
    [react/view styles/qr-code-viewer
     [status-bar/status-bar {:type :modal}]
     [qr-viewer-toolbar (:name contact) value]
-     [qr-code-viewer/qr-code-viewer {:style styles/qr-code}
-      value (i18n/label :t/qr-code-public-key-hint) (str value)]]))
+     (highlight/highlight-provider (fn [highlight-address]


Why not regular reagent component usage?

jeluard · 2018-04-27T13:21:43Z

src/status_im/ui/components/highlight.cljs

+  (let [highlighting (reagent/atom false)]
+    [react/touchable-without-feedback {:on-press   #(reset! highlighting false)}
+     [react/view {:flex 1}
+      (render-fn highlighting)]]))


Can't this be a regular reagent component composition?

jeluard · 2018-04-27T13:22:56Z

@go1t Thanks for the contribution! I added some comments.
It looks to me the highlight part could be simplified using plain reagent component?

go1t · 2018-04-28T22:54:46Z

@jeluard Thanks for the review!

Before the latest commit it was using plain reagent components (see here: https://github.com/status-im/status-react/pull/3957/files/f83415f0e698ec804d5ec4a909dd74b35e09b136). As you can see I had to copy and paste the atom and touchable-highlight with unhighlighting behavior everywhere I need to use it. I thought it would be nicer to abstract that part out and just expose the highlighting state to the outside so that it's easier to add / remove.

Does this justify its existence a little bit, or do you prefer the plain version more?

jeluard · 2018-05-02T06:36:13Z

@go1t You can create your function that encapsulate those, then use it with reagent square bracket syntax.
Something like (defn highlight-provider [props children]) then directly use [highlight-provider {}].

As a rule of thumb most components should follow this convention. Probably you can provide the highlight fn part of the props map.

Does that make sense?

go1t · 2018-05-02T23:08:47Z

Ah I see what you meant now. I wasn't aware that what I was doing was akin to defining a function that returns a component instead of defining a react component (sorry a little new to clojurescript and re-agent hehe). But yeah you are right my intention was to define a component that has render as a prop. I will update the pull request soon!

go1t · 2018-05-04T09:36:42Z

@jeluard I just force-pushed the same branch so I don't have to open a new pull request. Hopefully that works

jeluard · 2018-05-04T10:42:32Z

@go1t It does! Thanks for the extra effort.

go1t · 2018-05-04T10:43:41Z

No problem! Thanks for the review in the previous iteration.

statustestbot · 2018-05-04T10:53:39Z

75% of end-end tests have passed

Total executed tests: 16
Failed tests: 4
Passed tests: 12

Failed tests (4)

Click to expand

1. test_public_chat
Tap on SendMessageButton Looking for full text: '/command' E selenium.common.exceptions.WebDriverException: Message: An unknown server-side error occurred while processing the command. Original error: io.appium.uiautomator2.common.exceptions.UiAutomator2Exception: Timed out after 10000 milliseconds waiting for root AccessibilityNodeInfo at io.appium.uiautomator2.model.XPathFinder.getRootAccessibilityNode(XPathFinder.java:246) at io.appium.uiautomator2.model.XPathFinder.refreshUiElementTree(XPathFinder.java:224) at io.appium.uiautomator2.model.XPathFinder.getNodesList(XPathFinder.java:116) at io.appium.uiautomator2.handler.FindElement.getXPathUiObject(FindElement.java:78) at io.appium.uiautomator2.handler.FindElement.findElement(FindElement.java:153) at io.appium.uiautomator2.handler.FindElement.safeHandle(FindElement.java:102) at io.appium.uiautomator2.handler.request.SafeRequestHandler.handle(SafeRequestHandler.java:56) at io.appium.uiautomator2.server.AppiumServlet.handleRequest(AppiumServlet.java:208) at io.appium.uiautomator2.server.AppiumServlet.handleHttpRequest(AppiumServlet.java:199) at io.appium.uiautomator2.http.ServerHandler.channelRead(ServerHandler.java:44) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:366) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:352) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:345) at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:102) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:366) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:352) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:345) at io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.fireChannelRead(CombinedChannelDuplexHandler.java:435) at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:267) at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:250) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:366) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:352) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:345) at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1294) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:366) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:352) at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:911) at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:131) at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:611) at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:552) at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:466) at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:438) at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:140) at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:144) at java.lang.Thread.run(Thread.java:761) Device sessions: Device 1 Device 2
2. test_send_eth_to_request_in_group_chat
Wait for HomeButton Looking for IHaveAccountButton `E selenium.common.exceptions.NoSuchElementException: Message: 'IHaveAccountButton' is not found on screen, using: 'xpath://android.widget.TextView[@text='I ALREADY HAVE AN ACCOUNT']'` Device sessions: Device 1 Device 2
3. test_send_eth_from_wallet_sign_later
Tap on SignTransactionButton Looking for GotItButton `E selenium.common.exceptions.NoSuchElementException: Message: 'GotItButton' is not found on screen, using: 'accessibility id:got-it-button'` Device sessions: Device 1
4. test_group_chat_messages_and_delete_chat
Tap on SendMessageButton Looking for an element by text: 'a_chat_54105013' `E selenium.common.exceptions.NoSuchElementException: Message: 'BaseButton' is not found on screen, using: 'xpath://*[@text="a_chat_54105013"]'` Device sessions: Device 1 Device 2 Device 3

Passed tests (12)

Click to expand

1. test_send_transaction_from_daap
Device sessions: Device 1
2. test_send_eth_from_wallet_sign_now
Device sessions: Device 1
3. test_one_to_one_chat_messages_and_delete_chat
Device sessions: Device 1 Device 2
4. test_send_eth_to_request_from_wallet
Device sessions: Device 1 Device 2
5. test_contact_profile_view
Device sessions: Device 1
6. test_network_switch
Device sessions: Device 1
7. test_transaction_send_command_one_to_one_chat
Device sessions: Device 1
8. test_browse_link_entering_url_in_dapp_view
Device sessions: Device 1
9. test_transaction_send_command_wrong_password
Device sessions: Device 1
10. test_send_eth_to_request_in_one_to_one_chat
Device sessions: Device 1 Device 2
11. test_send_stt_from_wallet_via_enter_recipient_address
Device sessions: Device 1
12. test_transaction_send_command_group_chat
Device sessions: Device 1

asemiankevich · 2018-05-04T12:30:29Z

@go1t would you mind please look at the issues outlined:

long tap is not working on android, for ex: open status -> go to profile -> tap show my contact code -> long tap it. Please, notice that the code is now hidden a bit under the picture
when I log tapped for ex the public key on iOS - tapping outside does nothing, the selection still remains as is

go1t · 2018-05-04T18:21:37Z

@asemiankevich Absolutely. Sorry I forgot to test on android before.

The 2nd issue is somewhat a behavior that we can't do much about unless we want to complicate things more (e.g. introducing an atom). I think tapping other elements such as the qr code image would make it disappear. I will see what I can do though.

go1t · 2018-05-05T10:01:16Z

Ugh seems like a multiline TextInput being non-selectable on android is another issue on react-native... :(

Since selectable works fine on android but buggy on iOS, while TextInput works fine on iOS but buggy on android, I think it might make sense to just use both of them (with https://facebook.github.io/react-native/docs/platform-specific-code.html). What do you think? @jeluard @flexsurfer

Platform Specific Code · React Native
When building a cross-platform app, you'll want to re-use as much code as possible. Scenarios may arise where it makes sense for the code to be different, for example you may want to implement separate visual components for iOS and Android.

jeluard · 2018-05-05T12:11:41Z

@go1t Please go ahead!

lukaszfryc · 2018-05-09T08:12:28Z

@go1t it works fine. One small issue that still persist is the following - only on iOS, when qr code is copied the text is still selected:

Can we deselect it? If not, let's merge.

flexsurfer · 2018-05-09T08:24:07Z

@go1t could you please squash commits, thanks

go1t · 2018-05-09T08:28:29Z

@flexsurfer will do!

@lukaszfryc there is no trivial way to deselect it as it's an upstream issue unfortunately and we would like to keep it simple so yeah I think we probably should go forward with this for now.

Signed-off-by: Andrey Shovkoplyas <[email protected]>

flexsurfer added the bounty label Apr 22, 2018

go1t changed the title ~~Allow address copying in the QR code viewer screen~~ Allow address copying in the QR code viewer, wallet & contact screens Apr 22, 2018

go1t force-pushed the feature/allow-address-copying-3616 branch 2 times, most recently from 55911b0 to 2a02b4f Compare April 22, 2018 23:12

status-im deleted a comment Apr 23, 2018

denis-sharypin requested review from jeluard, goranjovic and cammellos April 23, 2018 18:28

denis-sharypin requested review from alwx and flexsurfer April 26, 2018 06:33

jeluard suggested changes Apr 27, 2018

View reviewed changes

go1t force-pushed the feature/allow-address-copying-3616 branch from d256818 to e5d5c94 Compare May 4, 2018 09:35

goranjovic approved these changes May 4, 2018

View reviewed changes

jeluard approved these changes May 4, 2018

View reviewed changes

asemiankevich added the Tested - Issues label May 4, 2018

go1t force-pushed the feature/allow-address-copying-3616 branch from a18b849 to 0f9b717 Compare May 7, 2018 12:34

jeluard removed the Tested - Issues label May 7, 2018

jeluard approved these changes May 7, 2018

View reviewed changes

lukaszfryc self-assigned this May 9, 2018

lukaszfryc added the Tested - Issues label May 9, 2018

go1t force-pushed the feature/allow-address-copying-3616 branch from 0f9b717 to e82e57e Compare May 9, 2018 08:29

Make addresses in the QR code and the contact screen copyable

49dbfe1

Signed-off-by: Andrey Shovkoplyas <[email protected]>

flexsurfer force-pushed the feature/allow-address-copying-3616 branch from e82e57e to 49dbfe1 Compare May 9, 2018 08:32

flexsurfer merged commit 49dbfe1 into status-im:develop May 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow address copying in the QR code viewer, wallet & contact screens #3957

Allow address copying in the QR code viewer, wallet & contact screens #3957

go1t commented Apr 22, 2018 •

edited

Loading

ghost commented Apr 22, 2018

pablanopete commented Apr 23, 2018 •

edited

Loading

go1t commented Apr 23, 2018

flexsurfer commented Apr 26, 2018

go1t commented Apr 26, 2018

jeluard Apr 27, 2018

jeluard Apr 27, 2018

go1t May 3, 2018

jeluard May 4, 2018

jeluard Apr 27, 2018

jeluard Apr 27, 2018

go1t May 3, 2018

jeluard May 3, 2018

go1t May 3, 2018

jeluard Apr 27, 2018

jeluard Apr 27, 2018

jeluard Apr 27, 2018

jeluard commented Apr 27, 2018

go1t commented Apr 28, 2018 •

edited

Loading

jeluard commented May 2, 2018

go1t commented May 2, 2018

go1t commented May 4, 2018

jeluard commented May 4, 2018

go1t commented May 4, 2018

statustestbot commented May 4, 2018

asemiankevich commented May 4, 2018

go1t commented May 4, 2018

go1t commented May 5, 2018 •

edited by ghost

Loading

jeluard commented May 5, 2018

lukaszfryc commented May 9, 2018

flexsurfer commented May 9, 2018

go1t commented May 9, 2018

Allow address copying in the QR code viewer, wallet & contact screens #3957

Allow address copying in the QR code viewer, wallet & contact screens #3957

Conversation

go1t commented Apr 22, 2018 • edited Loading

Summary:

ghost commented Apr 22, 2018

pablanopete commented Apr 23, 2018 • edited Loading

go1t commented Apr 23, 2018

flexsurfer commented Apr 26, 2018

go1t commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeluard commented Apr 27, 2018

go1t commented Apr 28, 2018 • edited Loading

jeluard commented May 2, 2018

go1t commented May 2, 2018

go1t commented May 4, 2018

jeluard commented May 4, 2018

go1t commented May 4, 2018

statustestbot commented May 4, 2018

75% of end-end tests have passed

Failed tests (4)

Passed tests (12)

asemiankevich commented May 4, 2018

go1t commented May 4, 2018

go1t commented May 5, 2018 • edited by ghost Loading

jeluard commented May 5, 2018

lukaszfryc commented May 9, 2018

flexsurfer commented May 9, 2018

go1t commented May 9, 2018

go1t commented Apr 22, 2018 •

edited

Loading

pablanopete commented Apr 23, 2018 •

edited

Loading

go1t commented Apr 28, 2018 •

edited

Loading

go1t commented May 5, 2018 •

edited by ghost

Loading