Handle account-card component pressed state bg color #17234
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 (28)
|
J-Son89
approved these changes
Sep 8, 2023
| @@ -29,10 +29,11 @@ | |||
| only icon | |||
| [button {:icon-only? true} :i/close-circle]" | |||
| [_ _] | |||
| (let [pressed? (reagent/atom false)] | |||
| (let [pressed-state? (reagent/atom false)] | |||
There was a problem hiding this comment.
Should the default value be set by pressed?
We can always update later if we find issues with it anyway
| @@ -67,10 +70,13 @@ | |||
| :margin-horizontal 4}) | |||
|
|
|||
| (defn add-account-container | |||
| [theme metrics?] | |||
| [theme metrics? pressed?] | |||
| @@ -6,15 +6,17 @@ | |||
| [quo2.components.buttons.button.view :as button] | |||
| [quo2.components.markdown.text :as text] | |||
| [utils.i18n :as i18n] | |||
| [quo2.theme :as theme])) | |||
| [quo2.theme :as theme] | |||
There was a problem hiding this comment.
Let's require as quo.theme to avoid clash with theme prop
|
|
||
| (defn- loading-view | ||
| [{:keys [customization-color type theme metrics?]}] | ||
| (let [watch-only? (= :watch-only type) | ||
| empty-type? (= :empty type)] | ||
| [rn/view | ||
| {:accessibility-label :loading | ||
| :style (style/card customization-color watch-only? metrics? theme)} | ||
| :style (style/card customization-color watch-only? metrics? theme false)} |
There was a problem hiding this comment.
This function should take a map imo, once the props start to go beyond 3 a map is cleaner generally 👌
| (when (not empty-type?) | ||
| [:<> | ||
| [rn/view (style/separator watch-only? theme)] | ||
| (def user-account |
There was a problem hiding this comment.
Why is this a def now? 🤔
@ulisesmac wdyt?
There was a problem hiding this comment.
This is not a proper form-2 component, we should be using defn.
With def, the state is initialized once for the lifetime of the anonymous function. This doesn't allow the component to be used twice or more on the same screen because they'd be writing/reading the same state in memory.
With defn and the form-2 style, the state would be initialized on every mount. Quite different.
There was a problem hiding this comment.
Yes exactly as @ilmotta said, the state atom is always the same no matter if we try to mount/remount the component.
BTW, I think the component generated would be a form-1 one, IMO very interesting, because the only way to notice this is not correct is when when we remount the component and its state is preserved
ilmotta
reviewed
Sep 8, 2023
| theme on-press]}] | ||
| (let [watch-only? (= :watch-only type) | ||
| empty-type? (= :empty type) | ||
| account-amount (if (= :empty state) "€0.00" amount) |
There was a problem hiding this comment.
We should translate the empty value too because euro doesn't make sense for someone using another currency.
There was a problem hiding this comment.
I think these values will be updated in a separate pr anyway, we'll be soon to start interacting with the backend and will handle these properly then 👍
There was a problem hiding this comment.
Probably the default value will just get passed a prop
There was a problem hiding this comment.
I see, but quo components imo should be built independently of these details. If the backend isn't ready it shouldn't affect how we code the component.
By not following the guidelines for translations, we need to keep track in the future of fixing this code, because this can be seen as a bug for non-European users.
There was a problem hiding this comment.
sure, probably it's best to remove this value then @mmilad75 and instead add a default prop in place of "€0.00".
i.e
(fn [{:keys [state name ... empty-amount]}]
(let [watch-only? (= :watch-only type)
empty-type? (= :empty type)
account-amount (if (= :empty state) empty-amount amount)
or we just use amount prop but handle this externally.
We could of course handle this with translation strings as @ilmotta is saying too. So all suggestions welcome. It's just that language != currency selected so that's why I would think better to pull this from the backend 👍 (not directly in the component ofc)
There was a problem hiding this comment.
I think the approach you suggested with a default prop is even better @J-Son89 👍🏼 The way I see it is that it's better to have a component production ready when we merge it into develop, so that we keep develop more or less releasable at all times.
There was a problem hiding this comment.
I think we don't need empty-amount or anything related to it. We already have :type :empty which we can say that it's empty. We simply can show the :balance.
There was a problem hiding this comment.
I think we don't need
empty-amountor anything related to it. We already have:type :emptywhich we can say that it's empty. We simply can show the:balance.
If that's aligned with designs, sounds good :)
| :weight :medium | ||
| :style (style/account-name watch-only? theme)} | ||
| account-name] | ||
| (when watch-only? [icon/icon :reveal {:color colors/neutral-50 :size 12}])]] |
| :theme theme | ||
| :metrics? metrics?}] | ||
| [rn/pressable | ||
| {:on-press-in #(reset! pressed? true) |
There was a problem hiding this comment.
Whenever we can, it's worth more aggressively extracting functions outside the renderer. on-press-in and on-press-out only depend on pressed?, so they can be moved outside the renderer.
There was a problem hiding this comment.
Yes but it's a prop from pressable component. How can it be extracted from renderer?
There was a problem hiding this comment.
@mmilad75, the technique is the following:
The callback function #(reset! pressed? true) relies exclusively on pressed?, and pressed? is defined as a local state outside the renderer. This means every time the component re-renders, we're recreating a new function instance in memory that always does the same thing (it doesn't depend on dynamic values, only in a stable reference to an atom).
(def user-account
(let [pressed? (reagent/atom false)
on-press-in #(reset! pressed? true)
on-press-out #(reset! pressed? false)]
(fn [{:keys [state name balance percentage-value loading? amount customization-color type emoji
metrics?
theme on-press]}]
(let [...]
(if loading?
[loading-view
...]
[rn/pressable
{:on-press-in on-press-in
:on-press-out on-press-out
:style (style/card customization-color watch-only? metrics? theme @pressed?)
:on-press on-press}
...])))))| :on-press-out #(reset! pressed? false) | ||
| :style (style/card customization-color watch-only? metrics? theme @pressed?) | ||
| :on-press on-press} | ||
| (when (and customization-color (not= :watch-only type)) |
There was a problem hiding this comment.
(not= :watch-only type) -> (not watch-only?)
| :weight :semi-bold | ||
| :style (style/account-value watch-only? theme)} | ||
| balance] | ||
| (when metrics? |
There was a problem hiding this comment.
Components should be smaller if we can do it, this greatly helps Reagent perform at its best.
For example, the whole hiccup inside (when metrics? ...) does not need to re-render when name, balance, loading?, emoji, on-press or customization-color change.
| (when (not empty-type?) | ||
| [:<> | ||
| [rn/view (style/separator watch-only? theme)] | ||
| (def user-account |
There was a problem hiding this comment.
This is not a proper form-2 component, we should be using defn.
With def, the state is initialized once for the lifetime of the anonymous function. This doesn't allow the component to be used twice or more on the same screen because they'd be writing/reading the same state in memory.
With defn and the form-2 style, the state would be initialized on every mount. Quite different.
ilmotta
reviewed
Sep 8, 2023
| [text/text | ||
| {:weight :semi-bold | ||
| :size :paragraph-2 | ||
| :style (style/metrics watch-only? theme)} account-amount] |
There was a problem hiding this comment.
ilmotta
reviewed
Sep 8, 2023
| :customization-color customization-color | ||
| :icon-only? true} | ||
| :i/add]]) | ||
| (def add-account-view |
There was a problem hiding this comment.
This should be defn instead of def. Comment with more explanation https://github.com/status-im/status-mobile/pull/17234/files#r1319930535
ilmotta
approved these changes
Sep 8, 2023
There was a problem hiding this comment.
Looks good! Thank you for fixing problems introduced from past PRs.
@mmilad75, for next PRs, consider creating branches that have more than just the issue number. I don't know if we wrote this down somewhere, but it's something we agreed as a team to avoid.
mmilad75
force-pushed
the
milad/17081
branch
from
f2a10f8 to
a7bf024
Compare
44% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (19)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@mmilad75
|
81% of end-end tests have passedFailed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
|
|
hi @mmilad75 the e2e run is completed. The failures shown in e2e are not related to this PR |
richard-ramos
pushed a commit
that referenced
this pull request
Sep 25, 2023
Handle account-card component pressed state bg color
ibrkhalil
pushed a commit
that referenced
this pull request
Oct 1, 2023
Handle account-card component pressed state bg color
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 #17081
Platforms
Screen.Recording.2023-09-08.at.16.52.45.mov
Screen.Recording.2023-09-08.at.17.14.00.mov