Refactor some quo2 components to use best practices #16817
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 (24)
|
OmarBasem
approved these changes
Jul 31, 2023
| @@ -13,20 +15,17 @@ | |||
|
|
|||
| ;; TODO: this implementation does not support group display picture (can only display default group | |||
| ;; icon). | |||
| (defn group-avatar | |||
| (defn view-internal | |||
There was a problem hiding this comment.
missing private method declaration defn-
| @@ -6,7 +6,7 @@ | |||
| [react-native.core :as rn] | |||
| [utils.number])) | |||
|
|
|||
| (defn step-internal | |||
| (defn view-internal | |||
ilmotta
approved these changes
Jul 31, 2023
| :notification-up 2 | ||
| :search-with-label 8 | ||
| :search 6 | ||
| :scroll-to-bottom 6) |
There was a problem hiding this comment.
Better have a fallback to nil in case.
jo-mut
approved these changes
Jul 31, 2023
J-Son89
force-pushed
the
jc/best-practices
branch
2 times, most recently
from
ad45dbc
to
92152ff
Compare
J-Son89
force-pushed
the
jc/best-practices
branch
from
92152ff
to
9177ee0
Compare
78% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Passed tests (32)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @J-Son89 thank you for PR. Take a look a found issue: ISSUE 1: UI for 'Jump to' button is brokenSteps:
Actual result:UI for 'Jump to' button is broken Expected result:The collor for 'Jump to' corresponds to the selected color of the use |
|
@J-Son89 I've observed that this issue persists in the latest nightly build. If the current scope of the PR refactor includes addressing this issue, kindly make the necessary fixes. If not, I'm prepared to create a separate issue. Thank you! ISSUE 2: [Andorid] The scroll button remains visible after reopening a chatSteps to Reproduce:
Actual Result:Upon reopening the chat, the previously visible scroll button remains present even when the chat is opened in a manner that doesn't allow for scrolling downward. https://www.dropbox.com/scl/fi/wvm8oyjya6y7is9z0fg6x/scroll.mp4?rlkey=kkihxrdf2pzvj4yghidif8zv7&dl=0 Expected Result:The scroll button should be hidden when the chat is reopened since the chat is displayed in a mode where scrolling down is not feasible. Device:real device: Pixel 7a, Android 13 |
|
Hi @VolodLytvynenko - sure I can take a look at both of these. Will let you know! |
J-Son89
force-pushed
the
jc/best-practices
branch
2 times, most recently
from
2cbbd46
to
938752e
Compare
83% of end-end tests have passedFailed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (34)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
J-Son89
force-pushed
the
jc/best-practices
branch
from
938752e
to
9baf340
Compare
|
Hi @J-Son89 indeed, issue 1 is resolved after rebase. One additional issue related to jump to is found: ISSUE 3: 'Jump To' Button Appearance Inconsistent Between Light and Dark ThemesSteps to Reproduce:
Actual Result:The 'Jump To' button is displayed with dark shades on the light theme and with a light appearance on the dark theme Expected Result:The 'Jump To' button is displayed with light shades on the light theme and with a dark appearance on the dark theme ENVs:
|
vkjr
approved these changes
Aug 9, 2023
J-Son89
force-pushed
the
jc/best-practices
branch
from
9baf340
to
057a31d
Compare
|
apologies @VolodLytvynenko - should be addressed now 👍 |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (36)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
ibrkhalil
approved these changes
Aug 9, 2023
|
@J-Son89 thank you for your work. PR is tested and ready to be merged |
J-Son89
force-pushed
the
jc/best-practices
branch
from
057a31d
to
f30e5c6
Compare
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.
Updated a few quo2 components to use
viewas their exported name,to use style and view namespaces and to wrap some components in
with-themecontext HOC.To test - see pages using
tabs,dynamic-buttonandgroup tag,bannerandstepThere should really be minimal changes here as it's all just internal refactoring to code formatting more than anything else