[#17023] Share qr code variants #17736
Conversation
ulisesmac
requested review from
vkjr,
J-Son89,
jo-mut,
OmarBasem and
siddarthkay
| (h/describe "Fires all events for all types" | ||
| (letfn [(test-fire-events [props test-seq] | ||
| (doseq [{:keys [test-name event-name | ||
| callback-prop-key | ||
| accessibility-label]} test-seq | ||
| :let [event-fn (h/mock-fn)]] | ||
| (h/test test-name | ||
| (render-share-qr-code (assoc props callback-prop-key event-fn)) | ||
| (h/fire-event event-name (h/get-by-label-text accessibility-label)) | ||
| (h/was-called-times event-fn 1))))] | ||
|
|
||
| (h/describe "Profile" | ||
| (test-fire-events | ||
| {:type :profile} | ||
| [{:test-name "Text pressed" | ||
| :accessibility-label :share-qr-code-info-text | ||
| :event-name :press | ||
| :callback-prop-key :on-text-press} | ||
| {:test-name "Text long pressed" | ||
| :accessibility-label :share-qr-code-info-text | ||
| :event-name :long-press | ||
| :callback-prop-key :on-text-long-press} | ||
| {:test-name "Share button" | ||
| :accessibility-label :link-to-profile | ||
| :event-name :press | ||
| :callback-prop-key :on-share-press}])) |
There was a problem hiding this comment.
Seriously thinking about creating this as a macro instead of being a function, it reduced the code complexity a lot and we just need to describe the test using a map 🤔 It could be generalized and exposed in the helpers namespace
WDYT?
Jenkins BuildsClick to see older builds (16)
|
| (defn- network-colored-text | ||
| [network-short-name] | ||
| [text/text {:style (style/network-short-name-text network-short-name)} | ||
| (str network-short-name ":")]) | ||
|
|
||
| (defn- wallet-multichain-colored-address | ||
| [full-address] | ||
| (let [[networks address] (as-> full-address $ | ||
| (string/split $ ":") | ||
| [(butlast $) (last $)]) | ||
| ->network-hiccup-xf (map #(vector network-colored-text %))] | ||
| (as-> networks $ | ||
| (into [:<>] ->network-hiccup-xf $) | ||
| (conj $ address)))) |
There was a problem hiding this comment.
@OmarBasem you recently refactored this behaviour, here I just added another implementation of it. It doesn't need to set ^:key for the text nodes, you could take it in consideration while abstracting the general solution.
There was a problem hiding this comment.
@ulisesmac can you please show me how to make that work for the implementation in https://github.com/status-im/status-mobile/pull/17732/files/cd03a2349f9335a5aca623cb8212b45d3c4b5b6d#diff-e3dc7e8c9c9d0e79e3e3ef677ba5e72ebbf9fe1b8ceee3adf30bd08ff17fee62 In my implementation the address is a component with props. I think the one here is just text?
| [network-short-name] | ||
| {:color (-> network-short-name | ||
| (get-network-full-name :unknown) | ||
| (colors/resolve-color nil))}) |
There was a problem hiding this comment.
Why resolve nil? Better to pass theme if that is what's happening here
There was a problem hiding this comment.
This component doesn't have theme variants.
(resolver-color color nil) is being used because network colors don't have a dark/light variant and since this component is not "theme sensitive", then we don't have the theme prop available
There was a problem hiding this comment.
Got it, I think @ilmotta was suggesting we pass them so this is future proofed should we need to adjust the colors mechanism.
There was a problem hiding this comment.
Well, at least there's a way to quickly search for all calls to resolve-color where theme is nil, so it's not the end of the world. But I can see a designer changing some color to be themeable and they may expect the mobile code to automatically resolve colors by theme. Maybe they have this assumption and won't even bother telling us they changed this tiny detail. Or maybe this is overthinking on my part.
There was a problem hiding this comment.
I think if they add a dark color for the networks, they will also update figma to show light/blur variants of those components, IMO, right now we'd be creating code speculating. I'm not sure if it's worth it 🤔
If you think we should do it, then I can change it 🙂
| :blur-type :transparent | ||
| :overlay-color style/overlay-color | ||
| :background-color style/overlay-color}])] | ||
| [quo.theme/provider {:theme :dark} |
There was a problem hiding this comment.
I don't think we need the theme provider hardcoded here? It's just a case the we don't care about Thai component in light theme as we never use that
There was a problem hiding this comment.
Sorry, I don't get the idea.
I added this because all the styles and subcomponents in this component are expected to be rendered in dark mode regardless the current app theme, for example: texts, tabs and buttons.
I'm unsing this component in the preview screen (which can be dark or light), and we could use this component in any screen, so I think setting the theme/provider as dark is a task of this component.
There was a problem hiding this comment.
Hmm, we have not done this for other components. The point here is that we will never use this component in light theme anyway and even if someone is looking at the preview screens of light theme well they are looking at an incorrect configuration. Ideally tooling like Malli will handle this in the preview screens (maybe show an icon when using a misconfiguration or something ). It's a broader issue then 'theme'
I'm not sure we need to have this type of guard railing on the components.
There was a problem hiding this comment.
I used this approach here for the Scan QR screen
As you can see here the screen is forced to be dark, but the button should be light as per Figma designs https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=689-193111&mode=design&t=XRiavTx7gYh1t2PG-4
I can see other parts of the codebase where we are using the same approach, like here
There was a problem hiding this comment.
Just an fyi - this example
Is a bit different because it's a chunk of the screen which is in dark theme regardless of the theme of the page. I.e that's the bottom tabs in the application which is always in dark but the screen above it can be light/dark.
In the case of this component here- it will only be used on a page/wrapping with dark theme.
In any case I'm not strongly against the approach, just giving my 2c 👍
ulisesmac
force-pushed
the
17023-share-qr-code-variants
branch
from
945a0fd to
cb2dfa7
Compare
src/test_helpers/component.cljs
Outdated
| (defn get-rerender-fn | ||
| "Rerenders a component. | ||
| Takes a JS Object representing a component and returns a function that accepts hiccup | ||
| representing the component to rerender with, optionally, the new props. | ||
| e.g. | ||
| (let [rerender-fn (h/get-rerender-fn | ||
| (h/render [my-component {:prop-1 :A | ||
| :prop-2 :B}]))] | ||
| ;; Rerenders `my-component` with the new props | ||
| (rerender-fn [my-component {:prop-1 :new-A | ||
| :prop-2 :new-B}])) | ||
| " | ||
| [component] | ||
| (fn [component-updated] | ||
| (let [rerender-fn (oops/oget component "rerender") | ||
| react-element (reagent/as-element component-updated)] | ||
| (rerender-fn react-element)))) |
There was a problem hiding this comment.
Just a rerender function helper
There was a problem hiding this comment.
Nice @ulisesmac. The first sentence says "Rerenders a component", but that's not what the function does. For the first sentence we could use "Returns rerender function from React Native Testing Library.".
| :type :select | ||
| :options [{:key :profile} | ||
| {:key :wallet-legacy} | ||
| {:key :wallet-multichain}]}]) |
There was a problem hiding this comment.
I think it would be better if there is 2 types profile and wallet, and then when wallet is selected, legacy and multichain should be handled from within the component. When using the component in flows it is either the profile variant or the wallet variant (which defaults to legacy and the user can change it to multichain)
Basically, I think changing between legacy and multichain needs to be handled by user taps, not through external props. Currently in the preview screen I am not able to switch between legacy and multichain by tapping the buttons on the component.
There was a problem hiding this comment.
Hey @OmarBasem
We agreed on reflect the figma properties as mucha as we can.
In figma, the component receives a property type and the possible types are the ones I listed:
Currently in the preview screen I am not able to switch between legacy and multichain by tapping the buttons on the component.
Those tabs receive a callback, that callback could be used to mutate the a ratom that triggers a :type change 👍
| :padding-top 12 | ||
| :padding-bottom 8}) | ||
|
|
||
| ;; Header |
There was a problem hiding this comment.
For section comments, consider sticking to the Clojure style guide, i.e. 3 or 4 semicolons.
| [network-short-name] | ||
| {:color (-> network-short-name | ||
| (get-network-full-name :unknown) | ||
| (colors/resolve-color nil))}) |
There was a problem hiding this comment.
Well, at least there's a way to quickly search for all calls to resolve-color where theme is nil, so it's not the end of the world. But I can see a designer changing some color to be themeable and they may expect the mobile code to automatically resolve colors by theme. Maybe they have this assumption and won't even bother telling us they changed this tiny detail. Or maybe this is overthinking on my part.
| :size 24 | ||
| :active (= :wallet-multichain share-qr-type) | ||
| :on-press on-multichain-press} | ||
| "Multichain"] |
There was a problem hiding this comment.
Are we sure multichain is an internationally understood word? In portuguese I sometimes saw/heard the usage of the word multi-cadeia, so maybe we should translate.
There was a problem hiding this comment.
Yep, forgot to add them, now it's addded!
| (defn wallet-multichain-bottom | ||
| [{:keys [share-qr-type component-width qr-data on-text-press on-text-long-press | ||
| on-share-press networks on-settings-press]}] | ||
| (let [network-image-source (fn [network] |
There was a problem hiding this comment.
Nitpick: this function can be decoupled from the view lifecycle because it's not closing over any value. When it's defined inline, the dev is forced to read the entire function implementation to be sure it's not closing over the props. I don't know if others do this, but I often do in my attempts to understand if the component can be simplified.
There was a problem hiding this comment.
I don't always remove constant functions from component's life cycle unless they are complex, sometimes I just create them on the mount phase of the component.
I extracted this function :) so now it's defined using defn-, I think it's a good suggestion, makes no sense to recreate the exact same function again and again.
| (str $ qr-data))) | ||
|
|
||
| (defn share-qr-code | ||
| "Receives the following properties: |
There was a problem hiding this comment.
FYI: These comments about the types will be largely superseded by Malli schemas and then we will be able to use the docstring only for the things that are truly important to manually document. By next week we should be able to start using this capability already.
src/test_helpers/component.cljs
Outdated
| (defn get-rerender-fn | ||
| "Rerenders a component. | ||
| Takes a JS Object representing a component and returns a function that accepts hiccup | ||
| representing the component to rerender with, optionally, the new props. | ||
| e.g. | ||
| (let [rerender-fn (h/get-rerender-fn | ||
| (h/render [my-component {:prop-1 :A | ||
| :prop-2 :B}]))] | ||
| ;; Rerenders `my-component` with the new props | ||
| (rerender-fn [my-component {:prop-1 :new-A | ||
| :prop-2 :new-B}])) | ||
| " | ||
| [component] | ||
| (fn [component-updated] | ||
| (let [rerender-fn (oops/oget component "rerender") | ||
| react-element (reagent/as-element component-updated)] | ||
| (rerender-fn react-element)))) |
There was a problem hiding this comment.
Nice @ulisesmac. The first sentence says "Rerenders a component", but that's not what the function does. For the first sentence we could use "Returns rerender function from React Native Testing Library.".
| [utils.i18n :as i18n] | ||
| [utils.image-server :as image-server] | ||
| [utils.re-frame :as rf])) | ||
| (:require [clojure.string :as string] |
There was a problem hiding this comment.
It would be better IMO if the formatting differences for the ns macro are reverted in this PR. I see other files changed too. Maybe your editor is formatting automatically for you like this? I ask this because zprint is not currently configured to keep the first namespace aligned with :require, the tool actually respects whatever style we choose, which is a problem because we will keep introducing personal styling choices that only make rebasing more painful since one of the most common conflicts are in the ns macro.
There was a problem hiding this comment.
I did this because we have differences in the formatting style between zprint and clj-kondo (btw, also IntelliJ formats requires as clj-kondo, so I think zprint is not doing it properly). The formatting added is equally styled for everyone.
Anyway, I reverted them in 0eaec99, since I also don't like this kind of diffs.
There was a problem hiding this comment.
Yeah, we need to sort this out at once. zprint should be driving the formatting, not the other tools, but zprint should also be configured a little bit better for the team's taste for namespaces.
| :value "Ethereum, Optimism, Arbitrum and unknown"}]}]) | ||
|
|
||
| (defn- get-network-short-name-url | ||
| [network-kw] |
There was a problem hiding this comment.
Just a bit unconventional to add types to names in Clojure.
| [width] | ||
| (into [rn/view {:style style/dashed-line}] | ||
| (take (style/number-lines-and-spaces-to-fill width)) | ||
| (cycle [[line] [space]]))) |
There was a problem hiding this comment.
It's been a while since I last saw cycle. Nice!
ulisesmac
force-pushed
the
17023-share-qr-code-variants
branch
from
960033c to
0eaec99
Compare
84% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (38)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@ulisesmac thanx for the PR. No issues from my side. Ready for design review. cc @Francesca-G |
There was a problem hiding this comment.
Hello @ulisesmac!
We updated the sharing screen (and the QR component), as stated in the change-log discord channel message here
Maybe we should put this design review on hold until we have updated screens, wdyt? :)
|
@Francesca-G - this work was started before the changes hade been made/requested in the changelog. I think it's better to treat the updates as a separate issue as it will take some time to properly adjust them and there are other priorities in the wallet development. I believe @ulisesmac has also done some none ui improvements here that will be great to have merged in. |
There was a problem hiding this comment.
@J-Son89 @ulisesmac sure, no problem :)
The component looks good, only thing that doesn't match is the icon, as you can see here (implementation on the right)
Seems like the icon used for the implementation is the same that's going to be used in the following design: So I'll keep it, and I'll create an issue to address the new design!. Thanks! |
ulisesmac
force-pushed
the
17023-share-qr-code-variants
branch
from
0eaec99 to
ea538bf
Compare
|
@Francesca-G Here's the new issue: |
* Align docstring * Create share-qr-code component * Remove empty style file * Implement common.share-qr-code including call to media server * Add share-qr-code preview screen * Use share-qr-code component in shell's share screen * Add tests and some fixes
fixes #17023
Summary
This PR implements the Share QR code variants component
Add avatar to profile variant and supports blur on Android:
Wallet Legacy variant:
Wallet Multichain variant:
Review notes
The QR code generation depends on a call to our media-server and we don't want to call the media server inside quo components. So the code is structured in the following way:
Testing notes
For Design review and QA:
Besides the preview screen added, the previous component has been substituted by this new one in the shell:
NOTE: Please for design review, focus only on the share QR code component in the shell, since other components (like emoji hash) haven't been changed.
Platforms
Steps to test
status: ready