chore: remove uses of override theme prop #16570
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
Jenkins BuildsClick to see older builds (28)
|
J-Son89
commented
Jul 18, 2023
| @@ -112,13 +112,11 @@ | |||
| :component add-new-contact/new-contact} | |||
|
|
|||
| {:name :how-to-pair | |||
| :options {:theme :dark | |||
There was a problem hiding this comment.
I noticed these screens phone status bar (time/battery etc) was in the wrong color for the theme
J-Son89
requested review from
ulisesmac,
smohamedjavid,
rahulpsingh,
ajayesivan and
erikseppanen
rahulpsingh
approved these changes
Jul 24, 2023
ajayesivan
reviewed
Jul 25, 2023
| @@ -188,3 +188,5 @@ | |||
| (if (= type :password) | |||
| [password-input base-props] | |||
| [base-input base-props]))) | |||
|
|
|||
| (def input (quo.theme/with-theme input-internal)) | |||
There was a problem hiding this comment.
When we wrap the internal component with HOCs or export components from the quo2.core namespace, the doc string is not being forwarded to the newly defined function. I'm curious if there is a way to fix this.
internal-input has a doc-string |
But input doesn't |
|---|---|
(Not in the scope of this PR)
There was a problem hiding this comment.
Yep @ibrkhalil pointed out you add docstring on the outer layer and it works. Perhaps someone can create a follow up issue to move the docstrings, and maybe even clean a few up while they're doing it :)
There was a problem hiding this comment.
Even if we try that approach when we define the components in quo2.core like this(def input quo2.components.inputs.input.view/input), the doc string is not being preserved.
There was a problem hiding this comment.
since the docstring is metadata, it might be possible to forward it with functions like meta, with-meta, etc.
There was a problem hiding this comment.
@erikseppanen @J-Son89 @ajayesivan
Even if we add it as metadata, some IDEs/Editors (like IntelliJ) that perform static code analysis will not read the docstring.
ajayesivan
approved these changes
Jul 25, 2023
87% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (34)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
erikseppanen
approved these changes
Jul 25, 2023
|
Hi @J-Son89 Could you please rebase the PR and resolve existing conflicts? After that we can perform manual testing. Thanx! |
|
@VolodLytvynenko - this is ready now 👍 |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
hi @J-Son89 thank you for PR. No issues from my side. Ready to be 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.
one further step towards: #16400
updates the usage of theme prop in
user_avatar
documentation_drawers
input
recovery phrase
preview list
filter selector
segmented tab
status tags
and removes the usage of
override-themefor these components within the appTo Test:
This affects the theming of components so a quick walk through of the app and ensuring the theme is correct on components is correct should be enough here.