Refactor Bottom Sheet to use Theme Context
#16710
Conversation
smohamedjavid
requested review from
ilmotta,
flexsurfer,
briansztamfater,
J-Son89,
OmarBasem and
siddarthkay
Jenkins BuildsClick to see older builds (16)
|
| [quo2.components.markdown.text :as text] | ||
| [quo2.foundations.colors :as colors] | ||
| [react-native.core :as rn] | ||
| [quo2.components.drawers.action-drawers.style :as style])) | ||
| [quo2.theme :as theme])) |
There was a problem hiding this comment.
nit: @ilmotta and I noticed that this can clash with theme prop, it's safer if we start doing [quo2.theme :as quo.theme]
smohamedjavid
force-pushed
the
refactor/bottom-sheet-theme
branch
from
43f00ed to
3f6edbf
Compare
| @@ -16,58 +16,47 @@ | |||
| {:label "Show red options?" | |||
| :key :show-red-options? | |||
| :type :boolean} | |||
| {:label "Override theme" | |||
| :key :override-theme | |||
| {:label "Drawer theme" | |||
There was a problem hiding this comment.
hmm, let's just remove this option and take it from the system settings. @mmilad75 and I were discussing we could also put the system theme selector in the preview screens too? rather than in the preview menu as it makes it easier to experiment with.
There was a problem hiding this comment.
@J-Son89 - I think we will need this selector as it will help us test the scenario such as the user's screen in light mode, but the bottom sheet needs to be rendered in dark mode.
This is the same as how the bottom sheet is used in Activity Center.
There was a problem hiding this comment.
ah that makes sense. thanks for the explanation! :)
|
Great stuff @smohamedjavid! 😎 |
smohamedjavid
force-pushed
the
refactor/bottom-sheet-theme
branch
from
3f6edbf to
2192e22
Compare
|
there's one or two uses in the codebase of |
|
|
||
| (defn action-drawer | ||
| [sections] | ||
| [:<> | ||
| (doall | ||
| (for [actions sections] | ||
| (doall | ||
| (map action actions))))]) | ||
| (let [filtered-actions (filter some? actions)] |
There was a problem hiding this comment.
I had to filter as we receive some nil values. This is due to the usage of when like:
status-mobile/src/status_im2/common/home/actions/view.cljs
Lines 414 to 426 in 32f18c3
These need to be refactored to use cond as it's safer and won't produce nil if the condition fails.
Prior to this filter, the component checks the value on render:
Removed the usage. Thanks, @J-Son89! |
89% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (32)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@smohamedjavid thanx for the PR! Please take a look at the following issue ISSUE 1 Messages longtap context drawer has wrong color in dark modeOn screenshot below you can see wrong color of context drawer block which differs from the rest bottom sheet blocks
|
|
@smohamedjavid also, I think it would be helpful to ask @Francesca-G for review of this PR. WDYT? |
smohamedjavid
force-pushed
the
refactor/bottom-sheet-theme
branch
from
f96c2a9 to
67e881e
Compare
There was a problem hiding this comment.
@pavloburykh here's the Figma frame with the design review :)
|
@pavloburykh - Thanks for testing this PR! Regarding
Thanks for spotting this! I noticed that this list was using the I created an issue: #16722 to replace it. Meanwhile, I have pushed a colour fix to that |
Too late to comment :) We haven't addressed any design system updates or design reviews in this PR. This PR just updates the theme context for the bottom sheet. But, we got the review from @Francesca-G. |
|
@Francesca-G Thanks a lot for the quick review. Appreciate it! Before are my findings, For incorrect typography issues in the author and message preview. Additionally, I noticed that we haven't implemented the design on the left (chat preview item on action drawer) yet. The issue regarding icon color are due to incorrect component usage. I have updated the colour in the component for now. I have opened an issue to replace the usage with the Regarding author is not shown on top of the message in the bottom sheet. This is not related to the PR but it's an interesting bug. Thanks for spotting it! This happens when the user (sender/receiver) sends a message (there is no message from the same author above), and sends a couple of messages after that. On the chat, from the second message, we don't display the author as it's from the same author. This same component is being used in the preview of the bottom sheet. @pavloburykh - We can create a separate issue to fix this. Also, the bottom sheet causing issues with chat header blur is reproducible on nightly or latest develop. Not related to this PR changes. |
Done, I have logged a separate issue #16736 @smohamedjavid thanx for fixing issue 1. PR is ready for merge. |
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
smohamedjavid
force-pushed
the
refactor/bottom-sheet-theme
branch
from
09c93f2 to
fbedbd8
Compare
Add locked input component, tests, styles Add translations Add duration icons Remove extra code Use theme from context Add missing code Update styles Update gas icon (previous was not reacting to size change) Use text from components instead of rn/text Fix styling for transaction sheet preview, locked input & account selector components Fix purple 50 color since it doesn't match design Work on PR suggestions Fix style to be pixel-perfect Comment-in tests Fix style Add docs for locked-input component Remove extra code Fixed design discrepancies Fix font-weight Fix purple color in account selector Remove unused icons Fix linter Fix tests fix for airplane mode [161108] Optimize message styling when there's multiple mentions on top of each other (#16505) Fix failing mute till test (#16453) fix navigation to community from discover communities screen (#16702) Update version to 0.162.3 [#16703] The display name is not resolved in chats for user sender after relogin (#16704) Mute community * mute and unmute community status-im/status-go@dfdaa72...e6187ae * mute and unmute community and all community chats status-im/status-go@dfdaa72...3abc86e * updated statu-go status-im/status-go@dfdaa72...919123e * refactored mute chat drawer status-im/status-go@d3e650d...3af0b17 * refactored mute chat drawer status-im/status-go@dfdaa72...3af0b17 * fixing mute channels * fixed mute community channels * update community chats mute status status-im/status-go@dfdaa72...dc50ac2 * added mute and unmute community toast status-im/status-go@dfdaa72...c06f7a6 * unmute community when atleast one community channel is unmuted status-im/status-go@dfdaa72...e691c47 * updated status-go status-im/status-go@b2e56f5...c52718c * updated status-go version v0.162.5 [Fix] Scroll to bottom on editing message (#16630) This commit fixes (by skipping) the scroll to the bottom of messages when the user edits a message and sends it. Signed-off-by: Mohamed Javid <[email protected]> Refactor `Bottom Sheet` to use Theme Context (#16710) This commit updates "Bottom Sheet" to use the theme (for theme provider) provided on the bottom sheet args when dispatching. This will ensure the theme is passed down to its child components where it can consume and render based on the theme. Changes done: In Bottom Sheet: - Fix Bottom Sheet to use the correct background colour (neutral-95) for dark mode (as per Figma) - Fix the Icon colour for danger in light mode - Updated Quo2 Preview to provide an option for the bottom sheet theme In Action Drawer: - Refactor the Action Drawer component to consume theme context Signed-off-by: Mohamed Javid <[email protected]> Revert extra commits Revert extra commits Revert extra changes
Add locked input component, tests, styles Add translations Add duration icons Remove extra code Use theme from context Add missing code Update styles Update gas icon (previous was not reacting to size change) Use text from components instead of rn/text Fix styling for transaction sheet preview, locked input & account selector components Fix purple 50 color since it doesn't match design Work on PR suggestions Fix style to be pixel-perfect Comment-in tests Fix style Add docs for locked-input component Remove extra code Fixed design discrepancies Fix font-weight Fix purple color in account selector Remove unused icons Fix linter Fix tests fix for airplane mode [161108] Optimize message styling when there's multiple mentions on top of each other (#16505) Fix failing mute till test (#16453) fix navigation to community from discover communities screen (#16702) Update version to 0.162.3 [#16703] The display name is not resolved in chats for user sender after relogin (#16704) Mute community * mute and unmute community status-im/status-go@dfdaa72...e6187ae * mute and unmute community and all community chats status-im/status-go@dfdaa72...3abc86e * updated statu-go status-im/status-go@dfdaa72...919123e * refactored mute chat drawer status-im/status-go@d3e650d...3af0b17 * refactored mute chat drawer status-im/status-go@dfdaa72...3af0b17 * fixing mute channels * fixed mute community channels * update community chats mute status status-im/status-go@dfdaa72...dc50ac2 * added mute and unmute community toast status-im/status-go@dfdaa72...c06f7a6 * unmute community when atleast one community channel is unmuted status-im/status-go@dfdaa72...e691c47 * updated status-go status-im/status-go@b2e56f5...c52718c * updated status-go version v0.162.5 [Fix] Scroll to bottom on editing message (#16630) This commit fixes (by skipping) the scroll to the bottom of messages when the user edits a message and sends it. Signed-off-by: Mohamed Javid <[email protected]> Refactor `Bottom Sheet` to use Theme Context (#16710) This commit updates "Bottom Sheet" to use the theme (for theme provider) provided on the bottom sheet args when dispatching. This will ensure the theme is passed down to its child components where it can consume and render based on the theme. Changes done: In Bottom Sheet: - Fix Bottom Sheet to use the correct background colour (neutral-95) for dark mode (as per Figma) - Fix the Icon colour for danger in light mode - Updated Quo2 Preview to provide an option for the bottom sheet theme In Action Drawer: - Refactor the Action Drawer component to consume theme context Signed-off-by: Mohamed Javid <[email protected]> Revert extra commits Revert extra commits Revert extra changes
Summary
This PR updates
Bottom Sheetto use the theme (for theme provider) provided on the bottom sheet args when dispatching. This will ensure the theme is passed down to its child components where it can consume and render based on the theme.Changes done:
Bottom Sheet
Bottom Sheetto use the correct background colour (neutral-95) for dark mode (as per Figma)Action Drawer
Action Drawercomponent to consume theme contextTesting notes
Kindly test the areas where the bottom sheet is displayed such as Chat, Communities, ...etc.
For dark themed bottom sheet, Check Activity Centre
more/options, andVerification Requestbottom sheet.Platforms
status: ready