-
Notifications
You must be signed in to change notification settings - Fork 987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: step component #15711
feat: step component #15711
Conversation
7429955
to
7695f71
Compare
Jenkins Builds
|
[quo2.components.counter.step.style :as style] | ||
[quo2.components.markdown.text :as text] | ||
[react-native.core :as rn] | ||
[utils.number :as utils-number])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to just require utils.number and use its as utils.number without alias
@@ -0,0 +1,19 @@ | |||
(ns quo2.components.counter.step.--tests--.step-component-spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh actually I think we prefer the name space is
quo2.components.counter.step.component-spec
because we already have the separated folder for step
(h/describe "step component" | ||
(h/test "default render of step component" | ||
(h/render [step/step {} nil]) | ||
(-> (h/expect (h/get-by-test-id :step-component)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to use accessibility over a test ID, this is a best practice from the rtl team
value (utils-number/parse-int value) | ||
label (str value) | ||
size (count label) | ||
accessibility-label (or accessibility-label (keyword (str "step-" label)))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's preferable just to change
(keyword (str "step-" label)
to something like :step-counter
because it can be set with accessibility-label
prop anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth mentioning accessibility labels don't need to be dynamic in most cases, since Appium has decent query selectors. This is mentioned in the guidelines too https://github.com/status-im/status-mobile/blob/develop/doc/new-guidelines.md#accessibility-labels.
My personal approach so far has been to only add dynamic accessibility labels like (str "step-" label)
if they request during review.
resolve #15696 Signed-off-by: yqrashawn <[email protected]>
7695f71
to
17202e1
Compare
93% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (27)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
all fixed |
resolve #15696
status: ready