-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gesture section list #15727
Gesture section list #15727
Conversation
353e388
to
f2928b6
Compare
Jenkins BuildsClick to see older builds (8)
|
f2928b6
to
fce55e8
Compare
@@ -76,3 +76,25 @@ | |||
[gesture-flat-list (rn-flat-list/base-list-props props)]) | |||
|
|||
(def scroll-view (reagent/adapt-react-class ScrollView)) | |||
|
|||
;;; Custom gesture section-list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to add a newline between section comments and the functions, otherwise it looks like the comment is correlated specifically with the function implementation.
src/react_native/gesture.cljs
Outdated
[{:keys [sections render-section-header-fn render-fn] :as props}] | ||
(let [data (flatten-sections sections)] | ||
[flat-list | ||
(conj props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit more idiomatic to use merge
to merge maps instead of conj'ing them.
src/react_native/gesture.cljs
Outdated
@@ -76,3 +76,25 @@ | |||
[gesture-flat-list (rn-flat-list/base-list-props props)]) | |||
|
|||
(def scroll-view (reagent/adapt-react-class ScrollView)) | |||
|
|||
;;; Custom gesture section-list | |||
(defn flatten-sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: make this function private, so that this function won't be suggested when autocompleting from other files.
src/react_native/gesture.cljs
Outdated
;;; Custom gesture section-list | ||
(defn flatten-sections | ||
[input-vector] | ||
(reduce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of implementation, since we're flattening just one level, I tried to cook a solution with mapcat
. As far as I checked, the implementation is equivalent, but should perform better since it's considerably simpler. Also select-keys
is avoided, since that scans the entire map when we only need the title.
(defn flatten-sections
[sections]
(mapcat (fn [{:keys [title data]}]
(into [{:title title :header? true}] data))
sections))
Here's the test, the equality below shows they're equivalent. The only difference is that the final result of mapcat
is a lazy sequence, whereas the reduce
you used returns a vector. But since the result of flatten-sections
is passed down to flat-list
, it doesn't matter if it's a vector or not.
(let [sections
[{:title "Section A",
:data
[{:primary-name "Alice"
:public-key "0x1"}
{:primary-name "Bob"
:public-key "0x2"}]}
{:title "Section B",
:data
[{:primary-name "Carol"
:public-key "0x3"}
{:primary-name "Daniel"
:public-key "0x4"}]}]]
(= (flatten-sections-old sections)
(flatten-sections-new sections)))
;; Result
;; => ({:title "Section A", :header? true}
;; {:primary-name "Alice", :public-key "0x1"}
;; {:primary-name "Bob", :public-key "0x2"}
;; {:title "Section B", :header? true}
;; {:primary-name "Carol", :public-key "0x3"}
;; {:primary-name "Daniel", :public-key "0x4"})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ilmotta ! This one definitely looks simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome :) Nice work in creating this component @OmarBasem
updates updates
dc015b6
to
f01a9b2
Compare
48% of end-end tests have passed
Not executed tests (4)Failed tests (13)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (12)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
fixes: #15726
This PR implements a gesture section list to be used in bottom-sheet-screen