Quo Wallet/Approval-Label Component #20117
Conversation
ajayesivan
requested review from
ilmotta,
seanstrom,
briansztamfater,
clauxx,
J-Son89 and
OmarBasem
Jenkins BuildsClick to see older builds (8)
|
| :borderBottomStartRadius 16 | ||
| :borderBottomEndRadius 16}] |
There was a problem hiding this comment.
Can we use kebab-case here?
There was a problem hiding this comment.
I think we need to set hole view up properly, or else it's just weird 👽
There was a problem hiding this comment.
As far as I understand, Reagent doesn't automatically convert kebab-case to camelCase for nested maps. Therefore, we need to use camelCase here. I'm not sure about the best way to fix this.
cc: @ilmotta
There was a problem hiding this comment.
@ajayesivan have you tried using it with kebab-case? I believe it should work
[hole-view/hole-view
{:holes [{:x 0
:y 0
:height style/top-hole-view-height
:width container-width
:border-bottom-start-radius 16
:border-bottom-end-radius 16}]
There was a problem hiding this comment.
Yes. I tried and it doesn't work.
There was a problem hiding this comment.
Oh that's right.. we are trying to change the case of a nested value not the prop key
There was a problem hiding this comment.
@ajayesivan, @OmarBasem We could definitely solve this problem in an elegant (automatic) way, but having a general solution for this problem would likely mean a tradeoff in performance, which we're not very keen of. On the other hand, given these nested props rarely appear and could be in a limited allowlist of "transformable keys" and that transformed keys are cached, it could be that the solution provided by a custom Reagent compiler would perform just as well as the current solution now (btw there's a proper Reagent compiler protocol).
I think it's more a matter of effort than anything else. Given our priorities and that hole props don't appear very often in the codebase, it doesn't make much sense to invest our time on such improvements.
Also I would add, we need to be very cautious with improvements in Reagent because if we are going to replace hiccup eventually, it would be a waste of resources to couple ourselves even more with Reagent. Hey, not saying we will kill it, but just to keep in mind.
There was a problem hiding this comment.
it could be that the solution provided by a custom Reagent compiler would perform just as well as the current solution now (btw there's a proper Reagent compiler protocol).
Good to know!
I think it's more a matter of effort than anything else. Given our priorities and that hole props don't appear very often in the codebase, it doesn't make much sense to invest our time on such improvements.
@ilmotta that's right, I agree!
There was a problem hiding this comment.
@ilmotta @OmarBasem @ajayesivan
Just a side comment.
This problem would also be solved if we explore the idea I mentioned in:
| :width container-width | ||
| :borderBottomStartRadius 16 | ||
| :borderBottomEndRadius 16}] | ||
| :style {:margin-top -24} |
There was a problem hiding this comment.
What is -24? Can we define this number in some constant?
There was a problem hiding this comment.
24 is the height of the hole-view, and if we don't add the negative margin here when using the component, it will look like this:
Instead we want this:
I have moved the value to a constant.
Yep definitely get the design review for the quo components 🙏 |
|
|
||
| (defn view | ||
| [] | ||
| (let [state (reagent/atom {:status :approve |
| (let [theme (quo.theme/use-theme) | ||
| [container-width set-container-width] (rn/use-state 0) | ||
| on-layout (rn/use-callback #(set-container-width | ||
| (oget % :nativeEvent :layout :width)))] |
There was a problem hiding this comment.
can we avoid the use of oops, it tends to cause crashes a lot 😢
I think at one point we were considering moving away from it completely
There was a problem hiding this comment.
I don't recall having experienced crashes due to oops. @J-Son89, do you have an example code to share that caused a crash?
There was a problem hiding this comment.
I'm not that familiar with the library, for now, I have removed the usage.
There was a problem hiding this comment.
@ajayesivan We shouldn't remove oops. Our guidelines already contain the example https://github.com/status-im/status-mobile/blob/624593ec3542ea3f7bcd3f9c2dcc4abe930bd68c/doc/new-guidelines.md#javascript-interop
Although the guideline lacks an explanation (can be found in the oops README). ClojureScript advanced compilation can and will often cause really hard to debug problems if we're not careful. oops solves this problem. I faced one such problem in status-mobile and lost hours trying to pinpoint the cause.
There was a problem hiding this comment.
Updated! It looks like I'm a little rusty on the guidelines. I will refresh 💪
There was a problem hiding this comment.
Updated! It looks like I'm a little rusty on the guidelines. I will refresh 💪
💪🏼 Thanks Ajay!
There was a problem hiding this comment.
can we avoid the use of oops, it tends to cause crashes a lot 😢 I think at one point we were considering moving away from it completely
@ilmotta I agree, we've had problems multiple times for a property not being in the object, it's weird, but the library throws an exception instead of returning nil.
| @@ -0,0 +1,67 @@ | |||
| (ns quo.components.wallet.approval-label.view | |||
| (:require [oops.core :refer [oget]] | |||
There was a problem hiding this comment.
We have 15 usages of oops.core using :refer. But ~60 usages of the namespace using :as because our guideline recommends avoiding :refer https://github.com/status-im/status-mobile/blob/b358c06a3132692c705e12cfd417a764e869341d/doc/new-guidelines.md#requireimport. It's better if we can stay consistent.
| [text/text | ||
| {:size :paragraph-2 | ||
| :style (style/message theme)} | ||
| (i18n/label (status status-message) |
There was a problem hiding this comment.
Nitpick: it's quite common in Clojure that if the map is statically defined, thus can never, ever be nil, the args are reversed. In other words, the static map would be treated as a function and become the first argument. So reversed it would be (status-message status).
Same logic can be applied to (status-icons status).
There was a problem hiding this comment.
I have updated the code. Is there any benefit to doing it like this?
There was a problem hiding this comment.
I mentioned as a nitpick, but really, it's a matter of preference. There's no advantage, you could also just use (get status-icons status) and it would be nice. One difference is that (status-icons status) will throw an exception if status-icons is nil, so that's why this is done only with static maps or static sets (they can also work as functions as in (#{:a :b} :a)).
There was a problem hiding this comment.
Thanks for the explanation!
| (defn- view-internal | ||
| [{:keys [status customization-color token-value token-symbol | ||
| container-style button-props] | ||
| :or {customization-color :blue}}] |
There was a problem hiding this comment.
@ajayesivan, this can lead to surprising (buggy) results if we aren't careful. That's why we have a guideline in src/quo/README.md about how to treat default values https://github.com/status-im/status-mobile/blob/b358c06a3132692c705e12cfd417a764e869341d/src/quo/README.md#default-value-when-destructuring.
ajayesivan
force-pushed
the
20081-approval-label-component
branch
from
4a9d7bf
to
2fb28c8
Compare
|
@briansztamfater @J-Son89 @ilmotta Thanks for the review! I have updated the code based on your suggestions. Please review it when you have a chance. |
ajayesivan
force-pushed
the
20081-approval-label-component
branch
from
15e5651
to
faf9126
Compare
| @@ -0,0 +1,14 @@ | |||
| (ns quo.components.wallet.approval-label.schema) | |||
|
|
|||
| (def ?schema | |||
There was a problem hiding this comment.
The schema is so simple and it's like a documentation about how the component should be used. I remember @ulisesmac said recently he's not a fan of always having a separate schema file and I agree with him. It's different than styles namespace, the schema is an integral part of the component API and forcing devs to always jump to a separate file to read it has a downside.
No suggestion for change @ajayesivan Just sharing thoughts and I added a section [2024-05-21] Should quo schemas be always defined in a separate file? in this issue's description for a future discussion #18726
There was a problem hiding this comment.
Thank you @ilmotta for opening the discussion.
Yeah, IMO it could be just above of the (def view ...) line.
There was a problem hiding this comment.
One nice thing with separate files is that the lookup becomes really easy to find similar examples.
I can look up schema files and some key and then use these cases, e.g customisation-color.
It's not so much friction either way though
There was a problem hiding this comment.
I'll add to that discussion instead
There was a problem hiding this comment.
Agreed @J-Son89. There are upsides to separate files. It's no big deal, just another point we can come back later on to assess.
There was a problem hiding this comment.
I also prefer the separate file approach for schemas. In my workflow, it's easier to look up schemas, especially since we don't have schemas for all components. I don't have any strong preferences. I'm happy to go with whatever the team decides.
83% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
|
E2E failures are not related to the PR |
|
Thanks everyone for the review! @Francesca-G can you do a design review of this PR? Thank you! |
ajayesivan
force-pushed
the
20081-approval-label-component
branch
from
faf9126
to
6e6d2bd
Compare
fixes #20081
Summary
Implements the Wallet/Approval Label component
Testing notes
Manual QA can be skipped since this PR only adds a quo component and doesn't introduce any changes outside Quo Preview.
Platforms
Areas that maybe impacted
None
Steps to test
Quo Preview -> Wallet -> Approval Label
Screenshots
Android
iOS
status: ready