feat: add new theming mechanism #16191
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
J-Son89
requested review from
flexsurfer,
smohamedjavid,
Parveshdhull,
OmarBasem and
ibrkhalil
J-Son89
commented
Jun 5, 2023
src/quo2/with_theme.cljs
Outdated
|
|
||
| (defn ^:private f-with-theme | ||
| [component props & args] | ||
| (let [theme (theme/use-theme)] |
There was a problem hiding this comment.
create a with theme HOC to pass theme prop to all components using this.
J-Son89
commented
Jun 5, 2023
src/status_im2/navigation/view.cljs
Outdated
| (defn screen-wrapper | ||
| [{:keys [insets background-color component sheet? screen-theme]}] | ||
| (let [user-theme (quo2.theme/get-theme)] | ||
| [theme/provider (or screen-theme user-theme) |
There was a problem hiding this comment.
we wrap the app root in a theme provider
Jenkins BuildsClick to see older builds (57)
|
J-Son89
commented
Jun 5, 2023
| :on-scroll-to-index-failed identity | ||
| :on-end-reached #(rf/dispatch [:activity-center.notifications/fetch-next-page]) | ||
| :render-fn notification-component}]])))) | ||
| [theme/provider :dark |
There was a problem hiding this comment.
activity centre is also wrapped in a theme provider and set to dark.
There was a problem hiding this comment.
Quick Question: Since the app root is wrapped with the theme provider. Is there a way we can pass the theme key (:light or :dark) as screen params just like :sheet? and send it to the provider? If yes, that would reduce the wrapping of the screen component again with the specified theme key.
There was a problem hiding this comment.
yeah that sounds like a nice solution. Seems like it would work to me We can definitely try this approach out! :)
J-Son89
commented
Jun 5, 2023
| @@ -95,3 +95,5 @@ | |||
| :height size | |||
| :disabled disabled | |||
| :background-color background-color}])]])) | |||
|
|
|||
| (def view (with-theme/with-theme view-internal)) | |||
There was a problem hiding this comment.
an example use case of a wrapped component using with-theme HOC
|
More theming options would be awesome, especially if theres an option to define our own colors with some sort of color picker. Here's how Signal does it: |
OmarBasem
approved these changes
Jun 6, 2023
src/status_im2/navigation/view.cljs
Outdated
| [status-im2.common.bottom-sheet-screen.view :as bottom-sheet-screen] | ||
| [status-im2.common.theme.core :as theme] | ||
| [quo2.theme :as quo2.theme] | ||
| )) |
src/status_im2/navigation/view.cljs
Outdated
| [:f> bottom-sheet-screen/f-view component] | ||
| [component])] | ||
| (when js/goog.DEBUG | ||
|
|
There was a problem hiding this comment.
There is an extra empty line I think?
smohamedjavid
approved these changes
Jun 6, 2023
There was a problem hiding this comment.
I like this solution. It reduces a lot of override-theme key used in our codebase.
Great work! @J-Son89!
src/quo2/with_theme.cljs
Outdated
| (defn ^:private f-with-theme | ||
| [component props & args] | ||
| (let [theme (theme/use-theme)] | ||
| (tap> theme) |
There was a problem hiding this comment.
I guess you forgot to remove the tap>
src/status_im2/navigation/view.cljs
Outdated
| [rn/view {:style (wrapped-screen-style insets background-color)} | ||
| [inactive] | ||
| (if sheet? | ||
| [:f> bottom-sheet-screen/f-view component] |
There was a problem hiding this comment.
Not for this PR, Just a reminder, We need this solution for the bottom-sheet too. For instance, the How to scan bottom sheet is a dark theme by default, and the Request to join community sheet is user themed.
There was a problem hiding this comment.
Yep absolutely, tried to keep this pr focused on the bare minimum to get it working but you are right in that we will need to add it there too!
| :on-scroll-to-index-failed identity | ||
| :on-end-reached #(rf/dispatch [:activity-center.notifications/fetch-next-page]) | ||
| :render-fn notification-component}]])))) | ||
| [theme/provider :dark |
There was a problem hiding this comment.
Quick Question: Since the app root is wrapped with the theme provider. Is there a way we can pass the theme key (:light or :dark) as screen params just like :sheet? and send it to the provider? If yes, that would reduce the wrapping of the screen component again with the specified theme key.
ilmotta
reviewed
Jun 6, 2023
There was a problem hiding this comment.
I'm getting an exception right when the app starts in the onboarding. Tried make clean and all the incantations. Do you get this @J-Son89?
ERROR Error: No protocol method IDeref.-deref defined for type null:
This error is located at:
in quo2.components.buttons.button.button_internal (created by quo2.with_theme.f_with_theme)
| @@ -204,7 +204,7 @@ | |||
| (when disabled | |||
| {:opacity 0.3}))) | |||
|
|
|||
| (defn button | |||
| (defn button-internal | |||
There was a problem hiding this comment.
To make it "more internal", we can change to defn- (private).
src/status_im2/navigation/view.cljs
Outdated
| screens) | ||
| (keyword key)) | ||
| ;; "everything" needed for hot reload | ||
| _ (def --e everything) |
There was a problem hiding this comment.
I have no words to explain how I feel about this line... Not this PR's fault though
There was a problem hiding this comment.
I added this line 😅 , We were debugging, we should have cleaned the code before opening the PR
There was a problem hiding this comment.
So it's not needed? Will remove so
hmm perhaps I left out some details. I will look into it and make sure the branch is working alright 👍 |
|
@J-Son89, I think we need to come up with a solution that doesn't involve using status-im2 namespaces inside quo2. That's the reason why |
|
Oh yes, I noticed and was thinking we just make theming a module outside or
inside of quo2.
So either src/quo2/theming
Or src/theming (somewhere around this level) we can consider it as a module
to itself.
|
briansztamfater
approved these changes
Jun 8, 2023
|
@J-Son89, the branch works now on my machine. Great stuff coming in soon! In this gist https://gist.github.com/ilmotta/85c72b1a001cb06669527fe93e600edb I refactored the code to use the existing namespace In the gist I also renamed the var |
Awesome, yes I agree. We can do as a follow up! |
🚀 Great! So I think we should at least nail down the namespaces in this PR @J-Son89, to create the basic infrastructure. The diff can be applied in a matter of minutes, and since this PR still needs more reviews, we can save time by doing some changes now rather than in a follow-up PR that will require explanation and agreement. Let me know if you would like me to push those changes to this PR since you're working with more important stuff now. |
|
Yep for sure, please push any changes you feel best to this branch :) |
ilmotta
force-pushed
the
porto/theming
branch
2 times, most recently
from
884b0b2
to
e9f128b
Compare
I would love something like that too @du82. I remember having conversations about the fact that our existing technical solution is too brittle and it assumes there are only ever going to be two themes. Even though this is true for the current app design, we know users love to personalize their apps. So it's kind of unfortunate we didn't model the code in a way to support whatever set of colors, fonts, etc users may want. One of the goals of this PR is to move one step closer to having a truly themeable code and eventually UI. |
J-Son89
commented
Jun 20, 2023
| @@ -36,7 +36,7 @@ | |||
| "node-libs-react-native": "^1.2.1", | |||
| "qrcode": "^1.4.1", | |||
| "react": "18.0.0", | |||
| "react-dom": "^16.4.2", | |||
| "react-dom": "18.0.0", | |||
There was a problem hiding this comment.
@siddarthkay this pr updates react-dom to match react version, these should always be in sync 👌
There was a problem hiding this comment.
Ooh my bad, Thanks @J-Son89 !
|
Thanks for finding these issues @VladimrLitvinenko! I will address them and let you know when I have solved them all. This pr does not add anything additional, i.e everything should look as it currently does in develop. The only thing this does is change the underlying mechanism of how the code is reading what theme is being used. There should only be one exception where the activity centre notification buttons and swipe button should now be the correct color, as they were incorrect before. |
J-Son89
force-pushed
the
porto/theming
branch
3 times, most recently
from
c7ba524
to
be35e62
Compare
|
@VladimrLitvinenko, haven't made changes here just updating this branch with latest. It's ready to be tested whenever you have some available time :) |
64% of end-end tests have passedFailed tests (12)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (21)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
82% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (27)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @J-Son89 thank you for PR. No issues from my side. Can be merged |
|
Thanks for all the testing @VladimrLitvinenko!! |
codemaster115
pushed a commit
that referenced
this pull request
Jul 7, 2023
* chore: set react-dom to same version as react
Merged
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.
This pr serves two purposes. It partially fixes: https://github.com/orgs/status-im/projects/90?pane=issue&itemId=29571439
mostly a UI bug from not setting
override-propfor these buttons in activity centre. ( "accept + decline swipe action colors should match button colors" )However the main focus of this pr is to introduce a new approach to how provide the current theme across the application.
New theming mechanism
The current implementation is causing a few issues - There is no scoped theming, which means that for parts of the application which have one theme but other content available on that same screen are using a different theme. At this point it is mostly
shell theme, for example Activity Centre behaves this way. Activity Centre usesshell themewhile the page behind it uses theusers theme.The current solution to workaround this is to make use of the
override-prop. This becomes very tedious, is error prone and requires a lot of additional work to maintain and implement features using this approach. Additionally all calls to(theme/get-theme)are insufficient on their own as there is a requirement to check the value ofoverride-themeas well. Simply there are two sources of truth for the current theme.This approach leads to prop drilling
override-themethrough multiple components, and then further set this prop on every page which is using another theme.New Approach
Instead this pr makes use of React Context. This means that all that's needed is a React Context Provider at the outer scope of any page, component, modal (any React component really).
From there components can make a call to the
useContexthook and it will receive thevalueof theme from the nearest React Context Provider.Mostly we will use this at the App Root level and then over components such as Activity Center ( I think Profile too) etc
With this approach we can remove the need for
override-themeand we use a HOC to passthemedirectly as a prop to all Quo2 components for which they can usethemefrom there.This leads to a single source of truth and means all helper functions etc are easier to use as a result.
Further, if going forward we can define themes as a set of values we can use this approach to load in themes directly to the components 👍
For more information see:
https://react.dev/learn/passing-data-deeply-with-context
https://react.dev/reference/react/useContext