[#13690] Fix visibility status color #13747
Conversation
|
Hey @ilmotta, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Jenkins BuildsClick to see older builds (10)
|
Samyoul
requested review from
briansztamfater,
Parveshdhull and
flexsurfer
and removed request for
briansztamfater and
Parveshdhull
For more context, see: status-im#13747 (comment)
|
@ilmotta , could you please rebase you branch onto develop, and resolve conflicts if any, thank you |
ilmotta
force-pushed
the
fix/visibility-status-color
branch
from
79cb56a
to
99ce1dc
Compare
99% of end-end tests have passedFailed tests (1)Click to expandClass TestPublicChatBrowserOneDeviceMerged:
Passed tests (86)Click to expandClass TestRestoreOneDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestWalletManagementDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestSendTxDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestKeycardTxOneDeviceMerged:
Class TestEnsStickersMultipleDevicesMerged:
|
|
Hey @ilmotta, thank you for your contribution and for the fix! ISSUE 1: Missing colors for the online status indicator in the Profile view in the dropdown menuSteps:
OS: Android, iOS Actual result: video_2022-08-03_15-28-10.mp4Expected result: IMG_1193.MP4 |
Hey @qoqobolo, I'll most definitely try to fix this today. Thanks for finding this out. |
@qoqobolo, I just fixed the problem in commit a5ac96b I would like to call your attention to an existing bug in the Here's the problem: when the new design is enabled in the
|
Hi, thank you for the screenshot, looks like in the new UI, the visibility status drop-down is also not aligning. UPD: Looks like the status updates drop-down issue is introduced in #13717, as we now allow the switcher to draw on the status-bar, we also need to consider the status-bar's height for calculating the dropdown's position. Created issue for that #13758 For the issue about dot color in profile section, I think we can leave them for now, as we didn't redesigned this screen yet. |
|
Sounds good to me @Parveshdhull. Thanks for taking a look at the problem. |
Agree. We are not yet subjecting the new UI to comprehensive testing, as many parts are still WIP. |
Fixes #13690
Summary
This PR fixes the visibility status color of the dot icon.
Review notes
This PR fixes the issue by introducing a new state called
:ui/old?(true by default). Components that are directly impacted by the redesign can simply subscribe to the new state and react accordingly. Edit: thanks to reviewers, I could replace the new state with the existing atomutils.config.new-ui-enabled?.AFAIU, the current approach to redesign on top of the existing design is to duplicate functions, using the suffix
*-old. This seems to divert from re-frame's recommendation to derive the UI from the app db and also causes an explosion of duplicated functions throughout the codebase.The duplicate+suffix strategy also leads to unnecessary changes to the call tree. For instance, some functions are duplicated because their downstream functions return a different color, but in reality, these parent functions have the exact same behavior. A concrete example, the parent component
emoji-chat-icon-viewdepends onprofile-photo-plus-dot-view, but it doesn't care what the inner component returns, so why should it change in a breaking way (var rename)? Of course, there are cases where it might be better duplicating a function, but most of the*-oldones I've seen could avoid such in-place changes.If this new approach seems reasonable to y'all, perhaps future PRs could use the new subscription, which could help reduce the number of regressions while the redesign isn't finished.
Testing notes
For manually testing between the old/new design, you can dispatch the event below in a REPL:
Edit: Use the existing effect to toggle the new UI.
The video below demonstrates the app already fixed, with the toggle on/off and how it affects the visibility status icon.
Peek.2022-08-02.08-51.mp4
Platforms
Steps to test
To reproduce the bug:
The issue #13690 describes the steps to reproduce, but to summarize:
For QA purposes:
Status: ready