Remove old quo code from status_im2 namespace #17404
Merged
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
|
Testing lint in ci. I will request review once ready. |
Jenkins BuildsClick to see older builds (25)
|
ajayesivan
force-pushed
the
17273-remove-old-quo-code-from-status_im2
branch
2 times, most recently
from
5b3666d
to
823dac2
Compare
ajayesivan
commented
Sep 25, 2023
| @@ -31,5 +30,4 @@ | |||
|
|
|||
| (defn set-theme | |||
| [value] | |||
| (quo/set-theme value) | |||
There was a problem hiding this comment.
@Parveshdhull, it appears that this was introduced during the Refactor app theme management process. I am in the process of removing it as part of the refactoring. I wanted to inform you of this change and ensure that it does not impact any essential functionality.
There was a problem hiding this comment.
We need to put this quo/set-theme usage back to fix the issue shared by @VolodLytvynenko here: #17404 (comment). Otherwise, the old UI elements will be broken until we fully migrate everything.
I can put this back and exclude this file from the linting script for now and create a follow-up issue to remove this usage later. @flexsurfer What do you suggest?
There was a problem hiding this comment.
let's do the hack for now, just set quo/set-theme to the atom and use atom here, so we could keep quo linting error, so nobody uses quo by mistake
There was a problem hiding this comment.
yeah i was wondering why you didn't remove quo.theme then, so it's still used
There was a problem hiding this comment.
@flexsurfer correct me if I'm wrong, but from my understanding, we can't start removing code until we remove all status_im usage from status_im2.
I didn't get what you meant by 'set quo/set-theme to the atom and use atom here'. Could you please elaborate?
ajayesivan
force-pushed
the
17273-remove-old-quo-code-from-status_im2
branch
from
823dac2
to
a4a3d62
Compare
ajayesivan
requested review from
flexsurfer,
Parveshdhull,
J-Son89 and
codemaster115
jakubgs
reviewed
Sep 25, 2023
jakubgs
reviewed
Sep 25, 2023
flexsurfer
approved these changes
Sep 25, 2023
Parveshdhull
approved these changes
Sep 25, 2023
ajayesivan
force-pushed
the
17273-remove-old-quo-code-from-status_im2
branch
from
3f02515
to
f759236
Compare
42% of end-end tests have passedFailed tests (25)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (18)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
jakubgs
approved these changes
Sep 25, 2023
20% of end-end tests have passedFailed tests (20)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
60% of end-end tests have passedFailed tests (17)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (26)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
From my understanding, the failing e2e tests are not related to this PR. Could someone from QA please confirm @status-im/mobile-qa? |
|
hi @ajayesivan currently, there are e2e failures occurring due to data delivery issues. I can quickly check manually to see if everything is alright. Could you please point out the possible areas that may be impacted by this PR? |
|
Hi @VolodLytvynenko, This PR removes the old quo usage in the new code and replaces it with the quo2 counterpart. I don't think this one requires any manual testing. I found 1 preexisting UI issue and reported it here: #17407. |
89% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Passed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
hi @ajayesivan one issue has been found that was introduced by this PR. The elements appear as light on the dark theme. This issue seems to affect only the old elements of the app, which I assume will be updated in the future. However, it looks a bit unattractive for now :) Is there something we can do to resolve this? Steps:
Actual result:
|
ajayesivan
force-pushed
the
17273-remove-old-quo-code-from-status_im2
branch
from
e651489
to
4ef5b5e
Compare
|
@VolodLytvynenko I have fixed the issue you reported. Could you please test again? Thanks! |
72% of end-end tests have passedFailed tests (12)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (31)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Hi @ajayesivan Thank you for fixed. Other issues are not related to this PR. Ready to be merged |
25% of end-end tests have passedFailed tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Passed tests (1)Click to expandClass TestActivityMultipleDevicePRTwo:
|
J-Son89
reviewed
Sep 28, 2023
| @@ -1,5 +1,5 @@ | |||
| (ns status-im2.common.qr-code-viewer.style | |||
| (:require [quo.design-system.colors :as colors])) | |||
| (:require [quo2.foundations.colors :as colors])) | |||
ajayesivan
force-pushed
the
17273-remove-old-quo-code-from-status_im2
branch
from
4ef5b5e
to
5927967
Compare
ibrkhalil
pushed a commit
that referenced
this pull request
Oct 1, 2023
clauxx
pushed a commit
that referenced
this pull request
Oct 3, 2023
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 #17273
status: ready