Malli schemas added to components #18949
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 (35)
|
|
As we discovered in other Malli pr's we can easily introduce changes that break the schema within the application code. Can we make it a standard practice for the developer to navigate to pages that use components with a new schema and check if everything is okay across all uses of the respective component? has to be done in a dev build ofc |
vkjr
force-pushed
the
list-items-schemas
branch
from
5fe5e90
to
9df4a90
Compare
ilmotta
approved these changes
Feb 23, 2024
ilmotta
reviewed
Feb 23, 2024
| @@ -102,7 +102,7 @@ | |||
| [details-view props]]] | |||
| [rn/flat-list | |||
| {:data accounts | |||
| :render-fn account-list-card/view | |||
| :render-fn (fn [item _rest&] [account-list-card/view item]) | |||
There was a problem hiding this comment.
Here we can just pass :render-fn account-list-card/view no? The ampersand is a bit weird, I've never seen it like that.
There was a problem hiding this comment.
That ampersand was a misprint, can be ignored, already fixed)
ilmotta
reviewed
Feb 23, 2024
| @@ -15,6 +15,6 @@ | |||
| :parent-height 560 | |||
| :animated? false}] | |||
| [rn/flat-list | |||
| {:render-fn quo/token-value | |||
| {:render-fn (fn [item & _rest] [quo/token-value item]) | |||
There was a problem hiding this comment.
@vkjr, could you expand on why are we wrapping the function token-value?
There was a problem hiding this comment.
It turned out that :render-fn receives bunch of arguments and value from data goes as a first argument, whereas all others cause schema error. So I had to leave them out.
There was a problem hiding this comment.
Since we have a recommendation to only pass non-anonymous functions to flat-list, I think it would be better to extract to a separate function. It's verbose, but at least we don't pay any cost in production runtime.
modified src/status_im/contexts/wallet/home/tabs/assets/view.cljs
@@ -5,6 +5,10 @@
[status-im.contexts.wallet.home.tabs.assets.style :as style]
[utils.re-frame :as rf]))
+(defn- token-value
+ [item & _]
+ [quo/token-value item])
+
(defn view
[]
(let [tokens-loading? (rf/sub [:wallet/tokens-loading?])
@@ -15,6 +19,6 @@
:parent-height 560
:animated? false}]
[rn/flat-list
- {:render-fn (fn [item & _rest] [quo/token-value item])
+ {:render-fn token-value
:data tokens
:content-container-style style/list-container}])))
ilmotta
reviewed
Feb 23, 2024
| [:percentage-change {:optional true} :string] | ||
| [:fiat-change {:optional true} :string]]) | ||
|
|
||
| (def ^private ?schema |
There was a problem hiding this comment.
Just saw there's a minor typo, missing colon to make it metadata. There are many instances of this typo in the PR. Sorry I missed this in previous review pass.
vkjr
force-pushed
the
list-items-schemas
branch
from
162b8fa
to
fde92e3
Compare
vkjr
force-pushed
the
list-items-schemas
branch
4 times, most recently
from
43f0c58
to
f411eb8
Compare
65% of end-end tests have passedFailed tests (16)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (31)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
|
J-Son89
approved these changes
Feb 29, 2024
OmarBasem
approved these changes
Mar 1, 2024
88% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@vkjr thanks for the PR. Failed e2e are not PR related |
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 #18353
Summary
Malli schemas added to following components:
Quo / List Item
Component tests fixed
status: ready