fix: polish new chat #19054
Merged
fix: polish new chat #19054
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
BalogunofAfrica
added
request-manual-qa
chat
BalogunofAfrica
requested review from
alwx,
seanstrom,
flexsurfer and
Parveshdhull
Jenkins BuildsClick to see older builds (64)
|
seanstrom
reviewed
Feb 29, 2024
| @@ -1,10 +1,12 @@ | |||
| (ns status-im.common.contact-list.view | |||
| (:require | |||
| [quo.core :as quo] | |||
| [quo.theme :as quo.theme] | |||
There was a problem hiding this comment.
tiny comment: we may be able to have just [quo.theme] instead of [quo.theme :as quo.theme]
src/react_native/gesture.cljs
Outdated
Comment on lines
99
to
114
| [{:keys [sections render-section-header-fn render-fn] :as props}] | ||
| [{:keys [sections sticky-section-headers-enabled render-section-header-fn render-fn] | ||
| :or {sticky-section-headers-enabled true} | ||
| :as props}] |
There was a problem hiding this comment.
I'm wondering 🤔
Is it safe to default sticky-section-headers-enabled to true? It seems this has not been supported until now, so maybe it needs to be an opt-in value instead of an opt-out. Thoughts?
There was a problem hiding this comment.
For section lists, sticky headers are enabled by default. This implementation uses FlatList under the hood (from gesture handler) because the normal section list from react native wouldn't work (the gestures do not work) in the context of a bottom sheet (this case).
There was a problem hiding this comment.
Ah I see, that makes sense, thanks for the explanation 🙏
flexsurfer
reviewed
Feb 29, 2024
flexsurfer
reviewed
Feb 29, 2024
J-Son89
reviewed
Feb 29, 2024
src/react_native/gesture.cljs
Outdated
| @@ -95,13 +95,25 @@ | |||
| (into [{:title title :header? true}] data)) | |||
| sections)) | |||
|
|
|||
| (defn find-sticky-indices | |||
There was a problem hiding this comment.
should we make it private?
There was a problem hiding this comment.
Yeah, makes sense to have it private
J-Son89
reviewed
Feb 29, 2024
| @@ -1,10 +1,12 @@ | |||
| (ns status-im.common.contact-list.view | |||
| (:require | |||
| [quo.core :as quo] | |||
| [quo.theme :as theme] | |||
There was a problem hiding this comment.
please use [quo.theme] as theme will class with the prop name
J-Son89
reviewed
Feb 29, 2024
| @@ -2,7 +2,7 @@ | |||
| (:require | |||
| [quo.core :as quo] | |||
| [quo.foundations.colors :as colors] | |||
| [quo.theme :as quo.theme] | |||
| [quo.theme :as theme] | |||
BalogunofAfrica
force-pushed
the
fix/polish-new-chat
branch
from
60359cd to
a9a47c4
Compare
12% of end-end tests have passedFailed tests (41)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
|
BalogunofAfrica
force-pushed
the
fix/polish-new-chat
branch
2 times, most recently
from
8f3eaf5 to
4dadb15
Compare
flexsurfer
reviewed
Mar 1, 2024
| [react-native.core :as rn] | ||
| [status-im.common.contact-list.style :as style])) | ||
|
|
||
| (defn contacts-section-footer |
| [rn/view (style/contacts-section-header (= index 0)) | ||
| [quo/divider-label title]]) | ||
| [{:keys [title]}] | ||
| (let [theme (quo.theme/use-theme-value)] |
There was a problem hiding this comment.
im not sure this is efficient, could you elaborate on why the header has a different color?
There was a problem hiding this comment.
The header has to have a background colour set when we want to use it as a sticky header.
If we do not set a background colour, we get this overlap of the header and the items when the header sticks to the top
There was a problem hiding this comment.
as we have our own implementation of section-list do you think if would be possible to set the theme as a prop to section-list and pass it to the header?
flexsurfer
approved these changes
Mar 1, 2024
BalogunofAfrica
force-pushed
the
fix/polish-new-chat
branch
3 times, most recently
from
20b6a07 to
6a54804
Compare
94% of end-end tests have passedFailed tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
BalogunofAfrica
force-pushed
the
fix/polish-new-chat
branch
from
88acac0 to
d98fdfd
Compare
|
@BalogunofAfrica I've checked your fixes and they are fine from my side. I move PR to design review by @Francesca-G . |
alwx
requested changes
Mar 14, 2024
src/react_native/gesture.cljs
Outdated
| (defn- find-sticky-indices | ||
| [data] | ||
| (->> data | ||
| (map-indexed (fn [index item] (if (:header? item) index nil))) |
There was a problem hiding this comment.
(if (:header? item) index nil) => (when (:header? item) index)
src/react_native/gesture.cljs
Outdated
| [data] | ||
| (->> data | ||
| (map-indexed (fn [index item] (if (:header? item) index nil))) | ||
| (filter (complement nil?)) |
There was a problem hiding this comment.
(remove nil?) instead of (filter (complement nil?)) has better readability
| [rn/view])]) | ||
| (let [customization-color (rf/sub [:profile/customization-color])] | ||
| [rn/touchable-opacity | ||
| {:on-press (when on-press on-press)} |
There was a problem hiding this comment.
just {:on-press on-press} will work fine as well
Francesca-G
approved these changes
Mar 14, 2024
There was a problem hiding this comment.
Looks good now ✨
@BalogunofAfrica thanks for addressing the other issues separately!
alwx
approved these changes
Mar 14, 2024
BalogunofAfrica
force-pushed
the
fix/polish-new-chat
branch
from
32d79e7 to
55673ea
Compare
This was referenced Jun 20, 2024
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 #18981
Fixes #19202
Summary
Polishes the new chat UI/Sheet
gesture/section-listwhich is implemented usinggesture/flat-list (Flatlist)fromreact-native-gesture-handlerPlatforms
Functional
Non-functional
Steps to test
status: ready