chore: remove colors/dark? method from colors file and update uses #17202
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
Jenkins BuildsClick to see older builds (32)
|
J-Son89
force-pushed
the
jc/theming-7
branch
2 times, most recently
from
fe641f0 to
f3d35b2
Compare
J-Son89
commented
Sep 6, 2023
| @@ -296,10 +296,6 @@ | |||
| (let [theme (or override-theme (theme/get-theme))] | |||
| (if (= theme :light) light dark)))) | |||
|
|
|||
| (defn dark? | |||
There was a problem hiding this comment.
🎯 removed this method. Imo it is odd to mix the colors and theming functionality/mechanisms like this. We should keep them separate in implementation and uses.
J-Son89
commented
Sep 6, 2023
src/status_im2/common/resources.cljs
Outdated
| @@ -88,6 +119,8 @@ | |||
| (js/require "../resources/videos2/generating_keys_02.mp4") | |||
| (js/require "../resources/videos2/generating_keys_03.mp4")]}) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
uff, will adjust empty lines
J-Son89
commented
Sep 6, 2023
| @@ -97,5 +130,5 @@ | |||
| (get ui k)) | |||
|
|
|||
| (defn get-themed-image | |||
There was a problem hiding this comment.
this method previously needed both keys, to me it seems better to use one key and have the themed options defined in the map. 👍 / 👎 ?
J-Son89
commented
Sep 6, 2023
| @@ -65,13 +69,17 @@ | |||
| :style style/authors-list}]]) | |||
|
|
|||
| (defn reaction-authors | |||
There was a problem hiding this comment.
it seems this file has many components exported. at the very least we should mark the defns that are not intended to be used outside as private to avoid later misuses. Or alternatively it is easier if each file has one exported component (view).
J-Son89
commented
Sep 6, 2023
| ([] (navbar nil))) | ||
|
|
||
| (defn statusbar | ||
| ([dark?] | ||
| (let [style (if (or dark? (colors/dark?)) :light :dark)] | ||
| (let [style (if (or dark? (quo.theme/get-theme)) :light :dark)] |
There was a problem hiding this comment.
uses of (quo.theme/get-theme) are temporary as I will address this properly in a separate pr. there's more pieces involved to change navigation code wrt to theming
There was a problem hiding this comment.
Shouldn't we change the condition to (or dark? (= :dark (quo.theme/get-theme))) since we are replacing (colors/dark?) with (quo.theme/get-theme)?
J-Son89
commented
Sep 6, 2023
src/status_im2/navigation/roots.cljs
Outdated
| @@ -94,7 +95,7 @@ | |||
| :children [{:component {:name :shell-stack | |||
| :id :shell-stack | |||
| :options (options/default-root | |||
| (if (colors/dark?) :light :dark))}}]}}} | |||
| (if (quo.theme/get-theme) :light :dark))}}]}}} | |||
There was a problem hiding this comment.
(= :dark (quo.theme/get-theme)) ?
There was a problem hiding this comment.
yes you are right, thanks @ajayesivan!
J-Son89
requested review from
briansztamfater,
ajayesivan,
OmarBasem,
alwx,
mmilad75 and
codemaster115
J-Son89
force-pushed
the
jc/theming-7
branch
from
f3d35b2 to
cc7b769
Compare
alwx
approved these changes
Sep 7, 2023
J-Son89
force-pushed
the
jc/theming-7
branch
from
cc7b769 to
d4b7e74
Compare
OmarBasem
approved these changes
Sep 11, 2023
37% of end-end tests have passedFailed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (16)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
ajayesivan
approved these changes
Sep 11, 2023
|
@J-Son89 can you please resolve the conflicts and rebase the PR? |
J-Son89
force-pushed
the
jc/theming-7
branch
from
d4b7e74 to
e917496
Compare
|
rebased 👍 |
J-Son89
force-pushed
the
jc/theming-7
branch
from
f46ce9f to
61766b6
Compare
J-Son89
force-pushed
the
jc/theming-7
branch
from
61766b6 to
4d5a3cf
Compare
60% of end-end tests have passedFailed tests (17)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
|
Hi @J-Son89. unfortunately, the issue is still reproduced in e2e |
J-Son89
force-pushed
the
jc/theming-7
branch
from
4d5a3cf to
7a0c5c9
Compare
|
@VolodLytvynenko - found the issue. I could reproduce it by long pressing on reaction buttons in chat. I have since fixed this issue. it was a simple fix in the code. apologies about that! |
74% of end-end tests have passedFailed tests (11)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (32)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
100% of end-end tests have passedPassed tests (3)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
@J-Son89 Thank you for the fixes. No issues from my side anymore @Francesca-G could you take a look?
|
@VolodLytvynenko should we tag @pedro-et here to check these since Franchesca is off this week? |
Hi @J-Son89 you are right. PR can be merged. No need for design review in this case |
J-Son89
force-pushed
the
jc/theming-7
branch
from
7a0c5c9 to
df49d72
Compare
| @@ -126,7 +126,7 @@ | |||
| (defn change-shell-status-bar-style | |||
| [] | |||
| (rf/dispatch [:change-shell-status-bar-style | |||
| (if (or (colors/dark?) | |||
| (if (or (quo.theme/get-theme) | |||
There was a problem hiding this comment.
(= :dark (quo.theme/get-theme))?
btw, quo.theme also has dark? function
There was a problem hiding this comment.
whoops, yes this needs to be updated! good spot @Parveshdhull
ibrkhalil
pushed a commit
that referenced
this pull request
Oct 1, 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.
a continuation of: #16400
This pr removes the function
colors/dark?as (imo) we should keep theming and colors seprate in functionality.There is no real functional changes here or UI changes. Everything changed is internal mechansims.
Also I adjusted the theme resources to be a little easier to use, i.e we only reference one key and pass theme as a prop to this method. I will leave a comment on this in pr
To test:
A few components have been adjusted:
Quo title input
https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?type=design&node-id=18490-198563&mode=design&t=OgIHdFbuaBLD9th1-4
Quo - Key card component - no uses in application
Quo record audio buttons -
Additionally a few of the chat screens are updated here.
The jump to page