Implement alert banner #19011
Implement alert banner #19011
Conversation
Jenkins BuildsClick to see older builds (60)
|
Parveshdhull
force-pushed
the
feat/banner-test-net
branch
10 times, most recently
from
909435d
to
f5153d7
Compare
Parveshdhull
requested review from
seanstrom,
flexsurfer,
smohamedjavid and
J-Son89
96% of end-end tests have passedFailed tests (1)Click to expandClass TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
🤩 Looking good! Thanks for setting this up 🙏 I do have a couple questions about some of the details, the main ones are:
|
| error-banner (:error banners) | ||
| safe-area-top (safe-area/get-top)] | ||
| [hole-view/hole-view | ||
| {:style {:background-color colors/neutral-100} |
There was a problem hiding this comment.
Not sure we should set this color permanently. Maybe we should check how it looks in light mode.
There was a problem hiding this comment.
Thank you for review Javid, I discussed this with mario and he mentioned alert banner will always use dark theme.
There was a problem hiding this comment.
In that case, this will affect the color of the status bar. Correct me if I'm wrong.
There was a problem hiding this comment.
In that case, this will affect the color of the status bar. Correct me if I'm wrong.
Thank you very much for highlighting this. I will keep this in mind while working for status-bar color implementation for #19072
|
Hi @seanstrom,
Yes, banners are not part of the screen. They are shown over all components.
I don't think they are affected 🤔 |
|
@Parveshdhull Thanks for the explanation, makes sense to me ✅ About the float action buttons, I suppose if their positioning is relative to the screen, then it should be okay as long as the screen dimensions are managed like they are in this PR (?) 🤔 |
Parveshdhull
force-pushed
the
feat/banner-test-net
branch
from
1fbd292
to
2a15359
Compare
pavloburykh
force-pushed
the
feat/banner-test-net
branch
from
19c236a
to
b86c28f
Compare
|
Hi @Parveshdhull ! Thanks fro your PR! Does my steps correct?
Also tried Goerli mode but it didn't helped. video_2024-03-07_12-34-06.mp4 |
|
Hi @mariia-skrypnyk, Thank you very much for taking PR in testing. PR only implements alert banner feature, but we are not yet using it. For checking out/preview the feature, please use these steps Settings -> Legacy settings -> Status IM components -> banner - > alert banner preview Testing notes
Manual testing is just a precautionary step to make sure merging this PR is safe. Thank you for pinging me, Sorry I forgot to update the description after completing this feature. Please feel free to ping if you have any more questions. |
Hi @Parveshdhull ! Thanks for such a detailed description! It was very useful! I found some minor things: ISSUE 1: Titles are not fully visible Place A) Advanced screen IMG_7270.MP4Place B) Notification settings screen Place C) Appearance screen Issues exist on both iOS and Android platforms. |
|
Thank you for reporting these issues. As old screens are using native topBar, custom margin are not working. This is expected and we can't fix that. But fortunately we are no longer using native topBar in new UI. These screens are all legacy and will be reimplemented, So please ignore these scenarios. |
|
Hi @Parveshdhull ! Thanks fro your answer. This issues are all that I've found during testing your PR. |
73% of end-end tests have passedFailed tests (12)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hi @Parveshdhull ! PR can be merged. |
Parveshdhull
force-pushed
the
feat/banner-test-net
branch
from
b86c28f
to
cc45a8c
Compare
Parveshdhull
force-pushed
the
feat/banner-test-net
branch
from
cc45a8c
to
38217ff
Compare
fixes #19007
output-2024-03-03_18.15.37.mp4
status: ready