[Refactor#18748] - Collectible Tag #18804
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 (19)
|
FFFra
force-pushed
the
refactor-#18748-Collectible-Tag
branch
from
9ac7397 to
fd60710
Compare
|
Incorporated changes from following PR #18799 |
clauxx
approved these changes
Feb 14, 2024
cammellos
approved these changes
Feb 14, 2024
FFFra
force-pushed
the
refactor-#18748-Collectible-Tag
branch
from
fd60710 to
7535c74
Compare
46% of end-end tests have passedFailed tests (25)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (22)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
shivekkhurana
requested changes
Feb 15, 2024
| (h/describe "Collectible_tag tests" | ||
| (h/test "Renders Default option" | ||
| (let [data (get-test-data {})] | ||
| (h/render-with-theme-provider [collectible-tag/view data] theme) |
There was a problem hiding this comment.
render-with-theme-provider doesn't need a default theme anymore. If no theme is passed, a default :light is assume automatically.
| [:=> | ||
| [:catn | ||
| [:props | ||
| [:map {:closed true} |
There was a problem hiding this comment.
We prefer open maps, because sometimes extra data might be passed down to components.
Malli maps are open by default, so removing {:closed true} would do.
There was a problem hiding this comment.
I don't fully agree with this, even though this was already merged. I think we should prefer closed maps for component props, unless needed for the "extra data" edge-case (which IMO shouldn't even happen 🥲). We had the conversation on discord a while back, but we didn't finish it and I think we should continue it someplace else, maybe an issue/discussion on GH, since on discord it's difficult to keep track of simultaneous convos.
There was a problem hiding this comment.
Yeah let's centralise it somewhere. Discord vote, notion doc or a GitHub discussion. Or anything like this 👌
| [:blur? {:optional true} [:maybe boolean?]] | ||
| [:theme :schema.common/theme] | ||
| [:collectible-img-src :schema.common/image-source] | ||
| [:collectible-name string?] |
There was a problem hiding this comment.
:string is preferred over string?. Same for boolean?, should be :boolean
Context: #18696 (comment)
| (case size | ||
| :size-24 10 | ||
| :size-32 12 | ||
| nil)) |
There was a problem hiding this comment.
I know you didn't make this change, but is an explicit nil needed ? Isn't that returned by default ?
There was a problem hiding this comment.
Ooh no, I'm pretty sure this is not right. Case will crash if no default value is provided so we must explicitly set it afaik
There was a problem hiding this comment.
Yes, @J-Son89. I saw it on google that nil is default so I though we were safe. But according to the docs:
https://cljs.github.io/api/cljs.core/case
If no default expression is provided and no clause matches, an Error is thrown.
Can I row back that?
There was a problem hiding this comment.
I think you should leave the explicit default. Afaik we've had this issue many times in the codebase 🧯
There was a problem hiding this comment.
@shivekkhurana do you also agree?
FFFra
force-pushed
the
refactor-#18748-Collectible-Tag
branch
from
7535c74 to
c70d7b2
Compare
FFFra
force-pushed
the
refactor-#18748-Collectible-Tag
branch
2 times, most recently
from
4e7445d to
333ee60
Compare
48% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (23)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
FFFra
force-pushed
the
refactor-#18748-Collectible-Tag
branch
from
aae4b66 to
579b87d
Compare
ilmotta
pushed a commit
that referenced
this pull request
Feb 15, 2024
[Refactor Collectible Tag] Improve component schema and add component_spec
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 #18797
closes #18799
Summary
Following the merge of the Quo Collectible Tag, it was observed that certain alerts were being triggered due to schema inconsistencies. This PR addresses those issues and incorporates
component_specsto enhance functionality.Simulator.Screen.Recording.-.iPhone.13.-.2024-02-07.at.14.11.21.mp4
Review Notes
This PR maintains the existing design and functionality.
Platforms
Functionalities
Steps to Test
Status: Ready