Use blurred background for all type shell? bottom sheets
#18400
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
ibrkhalil
requested review from
smohamedjavid and
ulisesmac
and removed request for
smohamedjavid
Jenkins BuildsClick to see older builds (56)
|
Comment on lines
109
to
113
| [blur/ios-view | ||
| {:style style/shell-bg | ||
| :blur-radius blur-radius | ||
| :blur-type :transparent | ||
| :overlay-color :transparent}]) |
There was a problem hiding this comment.
I'm afraid this would affect the sheets which use shell? (most sheets in onboarding do use the shell theme).
There was a problem hiding this comment.
I think I'll add a new prop for this blurring only.
Comment on lines
24
to
28
| {:icon :i/settings | ||
| :label (i18n/label :t/chat-notification-preferences) | ||
| :on-press (fn [] | ||
| (rf/dispatch [:navigate-to :notifications-settings]) | ||
| (rf/dispatch [:hide-bottom-sheet]))}]]])) |
There was a problem hiding this comment.
On the initial implementation, the Notification settings option is not added to the drawer (despite the Design showing the option) as the notification settings page is not designed/built yet.
The screen notifications-settings (which navigate to) is from legacy code. I'm not sure we need this option now.
There was a problem hiding this comment.
Okay, Will remove it.
ilmotta
reviewed
Jan 17, 2024
| [blur/ios-view {:style style/shell-bg}]) | ||
| [blur/ios-view | ||
| (cond-> {:style style/shell-bg} | ||
| blur-bg? (assoc :blur-radius |
There was a problem hiding this comment.
This assoc call is kind of unreadable with each argument on its own line instead of each key & value pair on each line.
There was a problem hiding this comment.
Maybe we can move the assoc next line.
| blur-bg? (assoc :blur-radius | ||
| blur-radius | ||
| :blur-type | ||
| :transparent |
There was a problem hiding this comment.
FYI: I can't verify the implications of forcing the blur type and overlay color as transparent because I don't have iOS.
There was a problem hiding this comment.
Would it be okay if QA validate it, And also I am going to try to test all sheets.
There was a problem hiding this comment.
A quick design question: If the sheet with the type shell? requires a transparent background, does it apply to all sheets with this type? If yes, we can update the style of this style.
There was a problem hiding this comment.
Got a response from design, Every shell? sheet should be blurred.
ibrkhalil
changed the title
shell? bottom sheets
smohamedjavid
approved these changes
Jan 29, 2024
jo-mut
approved these changes
Jan 29, 2024
|
@status-im/mobile-qa |
Hey @ibrkhalil! Please move PR from REVIEW to E2E column once it has been reviewed and approved. |
85% of end-end tests have passedFailed tests (5)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hey @ibrkhalil , thanks for the fix, looks great on iOS. This is how the AC bottom sheet looks on Android in this PR, the bottom sheet color is too dark:
However, afaik this is a known and common issue throughout the app (logged out state, user profile, etc):
Do you happen to have any updates on whether we should focus on this now? |
Yes, I can confirm the bottom sheet color is too dark on Android. It seems to also be connected to blur, a recurring problem with dark theme and blur effect throughout the app |
Yes, I believe this is the case. But maybe we could adapt the design (e.g. bottom sheet color) in some way until the blur issue is fixed? Or should we keep it as it is for now? |
@Francesca-G let me know what works the best for you? Thanks! |
|
Added this item for discussion on the Fit & Finish offsite: https://www.notion.so/Fit-Finish-Co-working-event-April-24-5d2edf9c37224488a32c314cf56059a2 |
|
Hi @ibrkhalil ! iOS looks good! So I move your PR to the design review for @Francesca-G approve. |
Francesca-G
approved these changes
Feb 13, 2024
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 #17571
Summary
Fixes the drawer style inside activity center
status: ready