[#15019] small option card component #15077
Conversation
ulisesmac
requested review from
ilmotta,
briansztamfater,
J-Son89 and
yqrashawn
Jenkins BuildsClick to see older builds (12)
|
There was a problem hiding this comment.
@flexsurfer, and devs we have icons2.. do we have any resources folder for new images etc? maybe it would be good to have images2.. wdyt?
| @@ -0,0 +1,51 @@ | |||
| (ns quo2.components.small-option-card.--tests--.small-option-card-component-spec | |||
There was a problem hiding this comment.
this can be in the name space
quo2.components.small-option-card..component-spec
also I just noticed you need to adjust the name spaces in quo2, they should match figma section and component name..
i.e in this case
quo2.components.onboarding-small-option-card.view
quo2.components.onboarding-small-option-card.style
quo2.components.onboarding-small-option-card.component-test
There was a problem hiding this comment.
Oh, thanks! I tried to follow the design's structure 😅
Solved in 3bb7d95
I moved them, except the last one suggested:
quo2.components.onboarding-small-option-card.component-test
Since there's no other test namespace ending with -test in the ns quo2.core-spec, they end with -spec:
(:require [quo2.components.banners.banner.component-spec]
[quo2.components.buttons.--tests--.buttons-component-spec]
[quo2.components.counter.--tests--.counter-component-spec]
[quo2.components.onboarding.small-option-card.component-spec] ;; <- I added this one
[quo2.components.dividers.--tests--.divider-label-component-spec]
[quo2.components.drawers.action-drawers.component-spec]
[quo2.components.drawers.permission-context.component-spec]
[quo2.components.markdown.--tests--.text-component-spec]
[quo2.components.selectors.--tests--.selectors-component-spec]
[quo2.components.selectors.filter.component-spec]
[quo2.components.record-audio.record-audio.--tests--.record-audio-component-spec]
[quo2.components.record-audio.soundtrack.--tests--.soundtrack-component-spec])Will the new component tests added not be inside a --test-- namespace?
| @@ -105,6 +106,9 @@ | |||
| {:name :dynamic-button | |||
| :insets {:top false} | |||
| :component dynamic-button/preview-dynamic-button}] | |||
| :cards [{:name :small-option-card | |||
There was a problem hiding this comment.
this should be :on-boarding it's mapped to the Figma design file to keep it familiar to navigate
There was a problem hiding this comment.
OK! it's solved in 3bb7d95
69% of end-end tests have passedFailed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (18)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
|
flexsurfer
changed the title
There was a problem hiding this comment.
It's hard to check if the source's uri passed to a fast-image is correct. so I'm just testing that the fast-image is rendered.
I actually think it's good that we don't test the URIs because it's not the component's responsibility to check their existence/validity. The component just knows "gives me a URI and I'll do my best to render that".
| (let [title "A title" | ||
| subtitle "A subtitle" | ||
| component-props {:title title :subtitle subtitle}] | ||
| (h/test "Title & subtitle are rendered - `:main` variant" |
There was a problem hiding this comment.
We can clearly see the test descriptions have duplicated prefixes to sort of "group tests", but JS test runners accept nested describes also for this very purpose. I think we should use this capability instead of duplicated prefixes.
In the future, we can also extend the h/describe macro to accept :only or :skip options, so we can easily narrow/skip blocks of tests (very common approach in Jest).
So my suggestion goes like this:
(h/describe "small-option-card"
(h/describe "Title & subtitle are rendered"
(let [title "A title"
subtitle "A subtitle"
component-props {:title title :subtitle subtitle}]
(h/test "`:main` variant"
(h/render (testing-small-option-card :main component-props))
(-> (h/get-by-text title) h/expect .toBeTruthy)
(-> (h/get-by-text subtitle) h/expect .toBeTruthy))
(h/test "`:icon` variant"
(h/render (testing-small-option-card :icon component-props))
(-> (h/get-by-text title) h/expect .toBeTruthy)
(-> (h/get-by-text subtitle) h/expect .toBeTruthy)))))There was a problem hiding this comment.
Cool! I'll do it, previously I tried to nest h/test but got an error 😅 thanks for the advice
There was a problem hiding this comment.
Solved in b28910f :)
| [rn/view {:style style/icon-variant} | ||
| [rn/view {:style style/icon-variant-image-container} | ||
| [fast-image/fast-image | ||
| {:test-ID :small-option-card.icon-image |
There was a problem hiding this comment.
I believe we preferred to use accessibility labels to find elements instead of :test-ID. @J-Son89? The accessibility labels also end up being useful for e2e tests in Appium, so it's a win-win if we can use them.
It would be nice to have this in the guidelines too. We already have a section on accessibility labels, so it's easy to add another sub section saying they should preferably be used for component tests instead of :test-ID.
There was a problem hiding this comment.
Yeah that's true actually, in this case it depends whether the devs of that package fast-image have provided accessibility label as a prop. We should add to the docs as you said.
There was a problem hiding this comment.
Ok, it''s fixed in 565e490
@ilmotta Well, I was trying to test that if the URI received is correctly used in the images we expect to use them. But thinking more about it, probably it is a redundant test. |
@ilmotta Is this ready to merge? |
ulisesmac
force-pushed
the
15019-small-option-card-component
branch
from
565e490
to
927fd41
Compare
Yes @ulisesmac, I approved 1h ago already. The code looks good to me. It's up to you if you want to merge it or if you prefer to wait/ask for feedback from other collaborators you assigned as reviewers. Given that the component does not affect production screens, it should be safe to merge. Sometimes it's a good idea to collect feedback from designers after the merge if you think the component has something to improve. It's a case by case decision of course :) |
Got it! thanks |
fixes #15019
Summary
Implements the small option card component in its two variants:
main&icon.Review notes
A new category in the
Preview Quo2.0 Componentssection has been added, it's called "onboarding":Credit to @J-Son89 for d83a14a. He did a PR (not merged yet) and I took his
mock-fn.Testing notes
Tests were added for this component. You can find them at
quo2.components.small-option-card.--tests--.small-option-card-component-spec.It's hard to check if the source's uri passed to a
fast-imageis correct. so I'm just testing that thefast-imageis rendered.These are the props that
fast-imagewith uri sourceresources/images/mock/small_opt_card_main.pnghas during testing:#js {:testID "small-option-card.main-image", :tintColor nil, :style #js {:position "absolute", :left 0, :right 0, :top 0, :bottom 0}, :source #js {:__packager_asset true, :width 100, :height 100, :uri "file://assets/full/path/to/directory/icon@2x.png", :scale 2}, :onFastImageLoadStart nil, :onFastImageProgress nil, :onFastImageLoad #object[Function], :onFastImageError #object[Function], :onFastImageLoadEnd nil, :resizeMode "contain", :children nil}That
uriinsidesourcekey is not the same I provided, so I can't use those props to check it.Please, share any ideas that you think would work to test if a
fast-imagecomponent is rendered using a specific uri source.Platforms
Areas that maybe impacted
Functional
Non-functional
Steps to test
status: ready