fix alert banner issues #19773
Merged
fix alert banner issues #19773
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
Jenkins BuildsClick to see older builds (56)
|
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
3 times, most recently
from
04d017d
to
0f8d63b
Compare
Parveshdhull
requested review from
alwx,
seanstrom,
flexsurfer,
J-Son89 and
ibrkhalil
ibrkhalil
approved these changes
Apr 24, 2024
| @@ -133,3 +133,7 @@ | |||
| (def community-accounts-selection-enabled? true) | |||
| (def fetch-messages-enabled? (enabled? (get-config :FETCH_MESSAGES_ENABLED "1"))) | |||
| (def test-networks-enabled? (enabled? (get-config :TEST_NETWORKS_ENABLED "0"))) | |||
|
|
|||
| ;; Alert banners are disabled for debug builds because alert banners overlay | |||
| ;; interfere with react-native debug tools, such as inspector and Perf monitor | |||
There was a problem hiding this comment.
+1
Makes total sense to disable it on debug builds
96% of end-end tests have passedExpected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (50)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
@Parveshdhull thank you for the PR. Please, take a look at the following issues. All of them are IOS related. Tested on iPhone X, IOS 16.7.7 ISSUE 1 Composer is overlayed by opened keyboard (IOS)telegram-cloud-document-2-5280547722179398306.mp4 |
ISSUE 2 Save button is overlayed by opened keyboard on Bio/Name screensSteps:
Actual result: telegram-cloud-document-2-5280547722179398290.mp4 |
ISSUE 3 Some legacy settings are overlayed by bannertelegram-cloud-document-2-5280547722179398322.mp4 |
|
Thank you @pavloburykh for testing the PR and finding these issues. Sorry I only tested android and missed these issues in IOS (for some reason my iphone takes around an hour to install PR builds) I will push fix for issue 1 and 2 soon. Issue 3 is expected(please check PR description) and overlapping of legacy screens can't be fixed, but I think we can get rid of black area. (will check that) |
ISSUE 4 Open keyboard overlays Confirm button on Edit Account Screen (IOS)Steps:
telegram-cloud-document-2-5280547722179398349.mp4 |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Thank you @Parveshdhull!
Sorry @Parveshdhull I have missed this comment.
I think it should be fixed in #19735 I will check after you rebase this PR after fixes to make sure. |
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
from
f38a378
to
c53ce23
Compare
|
@pavloburykh Thank you for finding these, issue 1, 2 and 4 should be fixed now. |
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
from
da0c63f
to
cd73cdb
Compare
@Parveshdhull thanks for the fixes. ISSUE 2 Save button is overlayed by opened keyboard on Bio/Name screensis fixed partially. Edit name screen looks fine but on Bio screen keyboard still overlays Save button on IOS. Please take a look. telegram-cloud-document-2-5282805973033961743.mp4 |
ISSUE 5 Expanded composer is overlapped by BannerReproducible both on IOS and Android Steps:
Actual result: expanded composer is overlapped by Banner. Unable to fold the composer by swipe. telegram-cloud-document-2-5282805973033961779.mp4 |
|
Thank you @pavloburykh for finding these issues, both issues should be fixed now. |
|
@Parveshdhull thank you for fixing those issue. Please take a look at another one ISSUE 6 Expanded composer is hardly foldable on IOSReproducible on IOS only. I am hardly able to fold expanded composer. Typically it should work by pulling the edge of the composer. Please, take a look how it behaves now: telegram-cloud-document-2-5287580984529670079.mp4 |
|
Issue 7 |
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
3 times, most recently
from
f7a507a
to
4e077b5
Compare
|
Thank you @pavloburykh for finding issue 6, it was an interesting one. Should be fixed now. About issue 7, I am not able to change the height of the native image picker for iOS, so for the time being I just hid the alert banner on this screen (only in iOS). We can further improve it later if needed, wdyt? |
Hey @Parveshdhull! Thanks a lot for fixing the issue. Looks good to me now!
I think hiding banner on this screen is a good a solution and we are fine to go with it, thank you. @Parveshdhull I have noticed another issue. Please take a look. I believe I can log it as followup if you prefer to get this PR merged first. ISSUE 8 Carousel with image preview is cut in gallery view modeAffects both IOS and Android Steps:
Actual result: image preview is cut. telegram-cloud-document-2-5294183719358186096.mp4Android:
IOS:
|
|
it doesn't look that it's worth it, should we just simplify the banner? cc @alwx |
Hey @flexsurfer! |
|
thank you @pavloburykh for finding issue 8, should be fixed now. UPD: component-tests failed will push a fix. |
|
upd2: failed component tests were not related to PR, rerunning worked |
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
from
fc8b1ae
to
1f0c8e7
Compare
Parveshdhull
force-pushed
the
fix/alert-banner-issues
branch
from
1f0c8e7
to
646ec7a
Compare
|
Thank you very much Pavlo for all your help and patience while testing the PR. |
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 #19506
Testing Note:
status: ready