[#16755] - Network routing component #17457
Conversation
ulisesmac
requested review from
briansztamfater,
clauxx,
J-Son89,
OmarBasem and
mmilad75
ulisesmac
force-pushed
the
16755-network-routing-component
branch
from
c827b5a to
f5b4fb2
Compare
Jenkins BuildsClick to see older builds (38)
|
|
A comment for reviewers: I've noticed this component has increaased its complexity A LOT just by adding the animations, its design is pretty simple but animations make a big change in the code and logic. So maybe we should create the components with animations included from the beginning, our delivery time will increase, but now I have the feeling that when we add a component without animations, we are adding a big technical debt. Anyway, I tried to make this code more readable, separate the animations in another namespace and give a meaninful name to them, I hope this PR is not too complex. 😅 |
| (js/setTimeout | ||
| (fn [] | ||
| (align-bars-off-screen {:new-network-values new-network-values | ||
| :network-bars network-bars | ||
| :amount->width amount->width}) | ||
| (reset-values-fn) | ||
| (js/setTimeout #(reset-bars-positions network-bars unlock-press-fn) 100)) |
There was a problem hiding this comment.
IMO we should store a reference and cancel timers before creating new ones, also cancel them when component unmounts
There was a problem hiding this comment.
They are very short living, but I agree 🤔 maybe we could create a common way to clean and set timeouts.
I've just added a mechanisms to track timeouts for this component and also to clean them once they have been used or just before we try to relaunch them.
Could you please check it and provide feedback?
thanks! :)
|
@ulisesmac thanks for your amazing work! This component is complex and required many animations, seems you came up with great results 🎉 Based on our requirements I think this PR is missing component tests. Given that this component is complex, testing for basic render and event triggering would be fine. I added some comments so far, but I'll try the branch locally so I can provide better feedback.
@ulisesmac In my opinion, it's fine to take extra time to add animations and improve the quality delivered. Of course, we should be smart enough to decide if it's worth the effort, and in this instance, I believe it's worth the effort. In summary, whenever we can raise the quality bar for our components, it's always appreciated. 💪 |
Thanks for your review @briansztamfater ! I will add component tests |
ulisesmac
force-pushed
the
16755-network-routing-component
branch
from
f5b4fb2 to
f2acbbf
Compare
|
@briansztamfater I solved the comments, could you please review again? |
ulisesmac
force-pushed
the
16755-network-routing-component
branch
from
49199d4 to
e439128
Compare
44% of end-end tests have passedFailed tests (24)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Passed tests (19)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Very good point @ulisesmac. In general, I've noticed in status-mobile that animated code often reduces the quality of the UX unless very carefully done. The opposite of higher quality. The added code complexity leads to bugs that are hard to catch, and there have been many fixes merged due to incorrect or bad animations. The increased frequency at which mutations are made to local state tends to reduce the overall performance of the app because more re-renders can be triggered. Also, animations tend (not always) to be highly subjective and can change in an instant if designers changes their mind, which is something I think we should be careful at the MVP stage because it causes rework. It would be a good idea to counter-balance these forces with a focus on performance from the get go too. In most PRs (not necessarily this one), I see performance as an after thought, even though there are many low hanging fruits. Performant code can't be sprinkled on top of existing code, just as animations can't be easily added on top. See Death by a thousand cuts. |
There was a problem hiding this comment.
@ulisesmac, I noticed some issues with the component, probably due to animations, which just adds to the comment #17457 (comment) I made that I think animations tend to lower the quality of the app .
1. Drag until the end between two sections and an exception is thrown.
bug.webm
2. Weird behavior
I don't understand the cause, but as I pressed on the component I started to see some animation artifacts, like the red section blinking first.
device-2023-10-03-085336.webm
3. The gap can be inconsistent at times
4. I don't know how, but I managed to see two sliders
5. Another animation artifact
Notice the border at the end looks weird after the animation finishes.
animation.bug.webm
ulisesmac
force-pushed
the
16755-network-routing-component
branch
from
f896f54 to
10f7c00
Compare
|
Merged. More animation issues will be reported in a separate issue with the video provided by designers |
fixes #16755
Summary
Implement the network routing component with its animation to select a new amount and supports any amount of networks.
Designs
Video of the animation
Supoort for different bars:
Video using 4 bars (sorry, bad video quality):
Screencast from 2023-09-29 19-13-22.webm
Testing notes
This PR is not implementing the tooltip on top of the bar since it's only presented in the animation video, not in the figma componet file. It'll be added separately.
Additionally, I'm faking a call to get new amounts for each network, so:
Platforms
Steps to test
status: ready