-
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
🧈 Transparent modal support 🧈 #15268
Conversation
:blur-type :dark}}]}]}) | ||
[{:keys [db] :as cofx} {:keys [filter-type filter-status]}] | ||
(rf/merge cofx | ||
{:db (cond-> (dissoc db :popover/popover) |
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.
You can remove the dissoc call here
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.
thx, fxd
Jenkins BuildsClick to see older builds (34)
|
Yeah, there's so much better in this PR that I'm using in my bread already 🧈 🧈 @flexsurfer, inset paddings are incorrect on Android. |
thanks @ilmotta how do you switch theme? in the app or in the system? |
@ilmotta real device or emulator? |
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.
Looks nice on iOS 👍
On Android, I am getting a black flicker on the background:
Screen_Recording_20230306-144155_Status.PR.mp4
Also, on Android, no notifications are rendered, as well as the empty view.
The top bar needs some padding. And please update the status bar style to light
.
Lastly, I see some screen's background color is broken now, so maybe put back the wrapped-screens-style
and customize it for the AC screen only. Would be good to run it through QA.
:modalPresentationStyle :overCurrentContext | ||
:layout {:componentBackgroundColor :transparent | ||
:backgroundColor :transparent}} |
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.
Hmm, I am wondering why it wasn't working..
@@ -39,8 +39,7 @@ | |||
(defn wrapped-screen-style | |||
[screen-insets safe-insets] | |||
(merge | |||
{:flex 1 | |||
:background-color (colors/theme-colors colors/white colors/neutral-100)} |
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 that's why
yeah Android is a pita, hope will fix all issues, thanks for testing |
You mean React Native is a pita ;) It should behave more similarly between platforms, but it doesn't. @flexsurfer, tested with released apk, all issues are present as in the emulator. |
That's bad news :( even a blue dead screen? |
Unfortunately yes @flexsurfer, and it happens on the newest version of Android as well as on the emulator. |
ok @ilmotta hope it is fixed now, at least i can't replicate on the emulator anymore ps i guess theme switch is not related to this PR, because i didn't make any related changes |
@OmarBasem pls take a look Android again when you have time, thanks |
@OmarBasem @ilmotta but please wait the latest build |
works perfect on my device and emulator, hope it will work for you as well @OmarBasem @ilmotta ready for testing |
5041944 build, android (no theme changes) |
oh thanks @Parveshdhull , yeah i removed color from the screen, i hate this theme changes, we should reload the app |
on android its very critical, we should set background color only once in the navigation settings, and never use background-color in nested views, i'll take a look how this can be fixed |
Yes, looks like this status-bar issue with activity center open-close also exists in develop (only able to reproduce in dark theme) |
About the animations, I did a try to use reanimated FlatList to do the layout animations. It should be trivial, but I faced a known bug in our current version of Reanimated and decided to park the implementation. I explained in the detail in this closed issue #14683 (comment). You might be able to find a solution though. Please, feel free to reopen the issue if you want to try your luck on it @OmarBasem, but do take a look at my comment there and the branch I pushed. |
known issue on Android Kureev/react-native-blur#511 |
Yeah, I've faced this problem from day 1 when using |
Hmm, why is it happening in the Modal Screen, but not the Modal component |
There are many subtle conditions that make the "bleeding" happen @OmarBasem. For instance, I've experimented (or seen in status-mobile, can't remember now) views that are not affected by bleeding because of animations (probably due to the delay to appear on screen), but this is just one example. |
i had a different bug , and when i set overlayColor to transparent this effect was more visible, so probably it depends somehow on different parameters |
yes, so @OmarBasem i looked at your PR, and you have |
so it only depends on color, brighter the blur color more visible bug effect |
this combination look more like a figma, and bug is not so visible |
Cool! Make sure the blur effect is still visible tho |
it looks pretty similar to figma, slightly visible |
3c0e4ff
to
c066f79
Compare
thanks @VladimrLitvinenko could you pls try latest build when its ready |
89% of end-end tests have passed
Failed tests (3)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (25)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
hi @flexsurfer. Thank you for the fixes. PR is tested and ready to be merged. |
on ios, on Android not relly :)
Screen.Recording.2023-03-06.at.11.19.27.mov