-
Notifications
You must be signed in to change notification settings - Fork 984
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
refactor: lightbox screen #15996
refactor: lightbox screen #15996
Conversation
(ns status-im2.contexts.chat.lightbox.utils | ||
(:require |
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.
This file is not a new code. These methods where initially in the view
file.
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.
I would have some comments for this file, but I'm not sure if it's worth sharing. I'm assuming the idea of this PR is to mostly move the code without many changes, is that correct?
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.
Yes correct. But feel free to share your comments (tho I am sure you reviewed this code before :) )
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.
I did, and you know @OmarBasem, code reviews are funny, it's like coding where the solution usually doesn't come out easily, requires trial and error. Similarly, in code reviews, as I familiarize myself with the new code (PR after PR being merged), the fog gradually dissipates.
Well, enough of comparisons!
Jenkins BuildsClick to see older builds (12)
|
57% of end-end tests have passed
Not executed tests (5)Failed tests (12)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (16)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
|
(ns status-im2.contexts.chat.lightbox.utils | ||
(:require |
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.
I would have some comments for this file, but I'm not sure if it's worth sharing. I'm assuming the idea of this PR is to mostly move the code without many changes, is that correct?
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.
Thanks for the PR! :)
I left some comments
7b3c33f
to
b88deae
Compare
d072a48
to
bb2a00a
Compare
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.
Thanks Omar!
Looks good
bb2a00a
to
e3d3bd0
Compare
Hi @OmarBasem, |
Hi @Parveshdhull this PR is just refactoring. E2e failed tests don't seem related to this PR. E2e tests a lot of times just fail, and then they pass on rerunning them. I ask for QA whenever needed. |
the tests are failing because of the icon change PR I believe. Ever since that one the app just crashes |
* refactor: lightbox screen
@OmarBasem please, next time if you're not sure about the reason why e2e failed, can you please ask QA for help? |
Sure thing! |
This PR fixes functional components usage in lightbox screen and does some refactoring.