quo2 documentation drawer component #15674
Conversation
Jenkins BuildsClick to see older builds (36)
|
| @@ -0,0 +1,27 @@ | |||
| (ns quo2.components.drawers.documentation-drawers.component-spec | |||
| (:require [quo2.components.drawers.documentation-drawers.view :as quo] | |||
There was a problem hiding this comment.
I think it would be more readable if you change to :as documentation-drawer?
| (defn view | ||
| "Options | ||
| - `title` Title text | ||
| - `cta?` Show CTA button |
There was a problem hiding this comment.
we don't need cta? prop, you can just do when on-press-cta ... below
There was a problem hiding this comment.
cta? is used to show/hide the button.
There was a problem hiding this comment.
yes but do we not only need the button when there is an on-press action 🤔
Maybe I have it wrong :)
There was a problem hiding this comment.
Yes, we can do that. In the component design it's a 'Read more' button and when clicked it will show the full content and hide the button. So I thought it's better to add a separate prop to control the visibility of the button. But we can achieve it using on-press, should I remove cta??
There was a problem hiding this comment.
Ah yea, you're right. Let's leave it as is so 👌
| "Options | ||
| - `title` Title text | ||
| - `cta?` Show CTA button | ||
| - `cta-label` CTA button label |
There was a problem hiding this comment.
consider something simple like "button" in place of cta. i.e
on-press-button
button-icon
| @@ -0,0 +1,10 @@ | |||
| (ns quo2.components.drawers.documentation-drawers.style) | |||
|
|
|||
There was a problem hiding this comment.
hmm, this is lacking the color variations? There should be light, dark and shell themes. 🤔 or you'll do that in a follow up?
There was a problem hiding this comment.
I'm using text, button and bottom-sheet components so dark & light variations will be handled by those components. For shell variant I have updated the bottom-sheet with a blur? option. Please check the the bottom-sheet/view file. Design only has shell variant in the dark theme so I think it will only be used in dark theme. Should we confirm this with the design team?
There was a problem hiding this comment.
Yes, that makes sense about the components.
shell theme is another variation of dark theme, let's confirm with them just to make sure it's all setup as expected 👍 .
| @@ -65,7 +65,7 @@ | |||
| {:opacity bg-opacity} | |||
| {:flex 1 :background-color colors/neutral-100-opa-70})}]] | |||
| ;; sheet | |||
| [gesture/gesture-detector {:gesture sheet-gesture} | |||
| [gesture/gesture-detector {:gesture sheet-gesture} ;; FIXME: This gesture handler is interfering with scrollview on android. | |||
There was a problem hiding this comment.
we use TODO and it should include a github link to that issue.
Unless this is just for your pr while it's in draft ofc :)
There was a problem hiding this comment.
It's just for me to fix before moving the PR to ready. I will use TODO for adding future tasks 👍
ajayesivan
force-pushed
the
15016-quo2-documentation-drawer-component
branch
3 times, most recently
from
96bd656
to
fb47b90
Compare
| @@ -12,7 +12,7 @@ | |||
| :margin-vertical 8}) | |||
|
|
|||
| (defn sheet | |||
| [{:keys [top bottom]} window-height override-theme] | |||
| [{:keys [top bottom]} window-height override-theme blur?] | |||
There was a problem hiding this comment.
you will probably need QA to review this pr since this adjusts the bottom sheet in app 👍
There was a problem hiding this comment.
@J-Son89 Do I just need to add the 'request-manual-qa' label for this, or is there anything else that needs to be done?
There was a problem hiding this comment.
Yes, once you have a dev approval (which you now do) then you can add that label . Also good to give a comment or update description of what area should be tested
ajayesivan
force-pushed
the
15016-quo2-documentation-drawer-component
branch
from
b6bf1c0
to
65de6f0
Compare
ajayesivan
force-pushed
the
15016-quo2-documentation-drawer-component
branch
2 times, most recently
from
92f74fc
to
87e7776
Compare
93% of end-end tests have passedFailed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (28)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
|
@ajayesivan Is it something expected for now? |
ajayesivan
force-pushed
the
15016-quo2-documentation-drawer-component
branch
2 times, most recently
from
94a3f7d
to
3849551
Compare
|
@churik Background color is correct. I have fixed the text color.
Shell theme background is the same on both light & dark themes. |
81% of end-end tests have passedNot executed tests (4)Failed tests (5)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (21)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@ajayesivan thanx for the fix. Ready to be merged. |
ajayesivan
force-pushed
the
15016-quo2-documentation-drawer-component
branch
from
3849551
to
32b7101
Compare
| @@ -21,9 +21,20 @@ | |||
| :right 0 | |||
| :border-top-left-radius 20 | |||
| :border-top-right-radius 20 | |||
| :overflow :hidden | |||
There was a problem hiding this comment.
This change caused bottom-sheet selected item to not show.
Fixed in #15677
fixes #15016
To test the component: quo2 preview -> drawers -> documentation drawers
Todo
Screencast iOS
documentation-drawer-ios-zip.mov
Screencast Android
documentation-drawer-android-zip.mov
Areas that may be impacted
This PR introduces a new blur background option - for the bottom sheets. Currently, we are only using this feature in the documentation drawer component(only added in quo2 preview). However, it would be wise to test other places where bottom sheets are used in our UI to ensure that this change does not impact the existing design and functionality.
status: ready