-
Notifications
You must be signed in to change notification settings - Fork 982
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
fix some buttons are not responding after theme change #14811
Conversation
Jenkins BuildsClick to see older builds (37)
|
36e24c3
to
545f1e7
Compare
70% of end-end tests have passed
Failed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestGroupChatMediumMultipleDeviceNewUI:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (16)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
|
maybe we should also improve safe-merge function, so that it will convert |
Hi @Parveshdhull could you resolve the conflicts, please? |
545f1e7
to
bc1aab3
Compare
yeah this one is tricky one, just wondering what if won't reset root, in new code it should work, just use hot-reload trick instead |
we can try, but not sure if it will work: https://wix.github.io/react-native-navigation/docs/style-theme#changing-theme-dynamically |
we don't use RNN themes, so it won't work, but hot reload might work, we just need to force re-render entire react tree |
tried hot reload, its working (apart from status bar icon, bottom bar colors, (easily fixable)). This is neat idea :) |
@VladimrLitvinenko, thank you for picking up PR. Please can you pause testing, I am thinking of adding few more changes to this PR. I will ping you once PR will be ready for testing. |
Sure. Let me know when it is ready |
bc1aab3
to
f08534e
Compare
9758e04
to
81ab015
Compare
1c4e1b9
to
7987732
Compare
Hi @VladimrLitvinenko, Thank you for your patience. PR is ready to be tested again. Apart from #14780, please also test the behavior of status bar icon colors, and nav bar colors as per system theme changes and also app theme changes. Known issueThese changes might create this distorted app state on old screens while changing theme. It's fine for now, we need to re-implement these screens without the RNN theme. But if distortion happening on new screens, please let me know. |
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.
nice
Hi @Parveshdhull , thanks for the fixes! ISSUE 1: Black font color of the status bar iconsCase 1:
In this case, the status bar icons have a black font with a black background from the beginning. Case 2:
You can see on the video, that in this case, the status bar looks okay during onboarding, but on the first video_2023-01-23_12-32-06.mp4On 14 sec you can also see that the
|
7987732
to
ee2bbf6
Compare
hi @qoqobolo, Thank you for testing the PR and finding this issue. Issue should be fixed now. Also, yes Appearance color is expected as this still uses RNN theme. |
Thanks @Parveshdhull ! ISSUE 2: The status bar icons get white on the white background (or vice versa) if switch the mode while a community channel is openSteps:
video_2023-01-23_17-10-59.mp4Also, ISSUE 1 (Case 2) is still reproducible on iOS. Steps:
|
Hi @qoqobolo, Issue 1: changing the status bar color without the re-initializing of app, is currently not working in ios. Issue 2: Currently we are not storing the ids of all opened screens, so on theme change we only change the status-bar icon color for the root screen. But once those screens closed and reopened the status bar color should be correct. As Issue 1 looks a little not fixable(needs further research) and Issue 2 non blocker, I think we can skip them for now and log them separately. wdyt? |
ee2bbf6
to
2896fe3
Compare
So, for ios, we are not able to change the status-bar icon color at runtime, but I made changes so that on login it should use the correct color. Please let me know if anything is not clear, It's a little confusing with so many edge cases. And thanks again for testing PR. |
0% of end-end tests have passed
Not executed tests (15)Failed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMediumMultipleDeviceNewUI:
Class TestActivityCenterMultipleDevicePR:
|
Sure, I'll log them in one new low-prio issue just to track them. But one more thing: e2e are failing because the video_2023-01-24_12-08-11.mp4 |
I also removed new-ui-toggle in PR and that accidentally exposed the else part(welcome screen). |
84% of end-end tests have passed
Failed tests (4)Click to expandClass TestGroupChatMediumMultipleDeviceNewUI:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (21)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Class TestActivityCenterMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Thanks @Parveshdhull ! |
679253f
to
2f7b6bf
Compare
fixes: #14780
Issue Cause
:close-chat
and:navigate-to-chat
both had:dispatch
key, it was causing:merging-fx-with-common-keys
error, due to unclosed chatstatus: ready