fix (fn[]) usage in hiccup #15713
Merged
fix (fn[]) usage in hiccup #15713
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 (51)
|
|
Is it worth it creating a macro for the f-COMP function? Maybe to have something like (defn my-comp [args]
(rf/create-functional-component :my-comp args)) |
yes, that was my first thought, but I'm not sure |
flexsurfer
force-pushed
the
feature/fix-fn-usage-in-hiccup
branch
from
c9f7a63
to
8ffa2ae
Compare
flexsurfer
changed the title
flexsurfer
requested review from
briansztamfater,
ilmotta,
Parveshdhull,
ajayesivan,
J-Son89,
OmarBasem,
ibrkhalil and
ulisesmac
|
cc @J-Son89 for components test |
Parveshdhull
approved these changes
Apr 24, 2023
ilmotta
approved these changes
Apr 24, 2023
There was a problem hiding this comment.
Great work, thanks @flexsurfer!
<rant>
It's so unfortunate that Reagent is causing this much trouble. I wish it could support functional components by default without this horrible :f> thing, just like newer libraries do, which were built for React hooks from the get-go, like Helix https://github.com/lilactown/helix/blob/master/docs/motivation.md. It's not the only problem in Reagent though...
</rant>
Is it worth it creating a macro for the f-COMP function?
Indeed @OmarBasem, this is quite common. In Helix there is the defnc aka defn component, which takes care of normalizing some stuff to help devs.
|
yeah, reagent needs some love from contributors, I feel like its poorly maintained after the author had abandoned it :( |
|
@flexsurfer, the pattern you established here is worthy of the guidelines. Could you please add a section there? |
0% of end-end tests have passedNot executed tests (4)Failed tests (25)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
its already here https://github.com/status-im/status-mobile/pull/15686/files#diff-b5e21cecfbe129756a13073f455833097d90938c8367957243f550fe8d76e567R39 |
Sorry I didn't explain correctly. I mean in this PR you named functional components with the prefix |
|
so, it seems like its better to use |
done |
flexsurfer
force-pushed
the
feature/fix-fn-usage-in-hiccup
branch
from
fa8c119
to
76d8561
Compare
ajayesivan
approved these changes
Apr 25, 2023
J-Son89
approved these changes
Apr 25, 2023
90% of end-end tests have passedFailed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (26)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
OmarBasem
reviewed
Apr 25, 2023
Comment on lines
209
to
273
| (let [{:keys [transparent? set-full-height?]} args | ||
| shared-element-id (rf/sub [:shared-element-id]) | ||
| exit-lightbox-signal (rf/sub [:lightbox/exit-signal]) | ||
| zoom-out-signal (rf/sub [:lightbox/zoom-out-signal]) | ||
| focused? (= shared-element-id message-id) | ||
| curr-orientation (or (rf/sub [:lightbox/orientation]) | ||
| orientation/portrait) | ||
| portrait? (= curr-orientation orientation/portrait) | ||
| dimensions (utils/get-dimensions | ||
| (or image-width c/default-dimension) | ||
| (or image-height c/default-duration) | ||
| curr-orientation | ||
| args) | ||
| animations {:scale (anim/use-val c/min-scale) | ||
| :saved-scale (anim/use-val c/min-scale) | ||
| :pan-x-start (anim/use-val c/init-offset) | ||
| :pan-x (anim/use-val c/init-offset) | ||
| :pan-y-start (anim/use-val c/init-offset) | ||
| :pan-y (anim/use-val c/init-offset) | ||
| :pinch-x-start (anim/use-val c/init-offset) | ||
| :pinch-x (anim/use-val c/init-offset) | ||
| :pinch-y-start (anim/use-val c/init-offset) | ||
| :pinch-y (anim/use-val c/init-offset) | ||
| :pinch-x-max (anim/use-val js/Infinity) | ||
| :pinch-y-max (anim/use-val js/Infinity) | ||
| :rotate (anim/use-val c/init-rotation) | ||
| :rotate-scale (anim/use-val c/min-scale)} | ||
| props {:pan-x-enabled? (reagent/atom false) | ||
| :pan-y-enabled? (reagent/atom false) | ||
| :focal-x (reagent/atom nil) | ||
| :focal-y (reagent/atom nil)} | ||
| rescale (fn [value exit?] | ||
| (utils/rescale-image value | ||
| exit? | ||
| dimensions | ||
| animations | ||
| props))] | ||
| (rn/use-effect (fn [] | ||
| (js/setTimeout #(reset! set-full-height? true) 500))) | ||
| (when platform/ios? | ||
| (utils/handle-orientation-change curr-orientation focused? dimensions animations props) | ||
| (utils/handle-exit-lightbox-signal exit-lightbox-signal | ||
| index | ||
| (anim/get-val (:scale animations)) | ||
| rescale | ||
| set-full-height?)) | ||
| (utils/handle-zoom-out-signal zoom-out-signal index (anim/get-val (:scale animations)) rescale) | ||
| (let [tap (tap-gesture #(on-tap portrait?)) | ||
| double-tap | ||
| (double-tap-gesture dimensions animations rescale transparent? #(on-tap portrait?)) | ||
| pinch | ||
| (pinch-gesture dimensions animations props rescale transparent? #(on-tap portrait?)) | ||
| pan-x (pan-x-gesture dimensions animations props rescale) | ||
| pan-y (pan-y-gesture dimensions animations props rescale) | ||
| composed-gestures (gesture/exclusive | ||
| (gesture/simultaneous pinch pan-x pan-y) | ||
| (gesture/exclusive double-tap tap))] | ||
| [gesture/gesture-detector {:gesture composed-gestures} | ||
| [reanimated/view | ||
| {:style (style/container dimensions | ||
| animations | ||
| @set-full-height? | ||
| (= curr-orientation orientation/portrait))} | ||
| [reanimated/fast-image | ||
| {:source {:uri (:image content)} |
There was a problem hiding this comment.
The component was previously two functions. It now has become one function, isn't this going to cause unnecessary re-renders?
And we now have a second let binding in the same function, which makes it redundant, right?
OmarBasem
reviewed
Apr 25, 2023
| (defn lightbox | ||
| (defn f-lightbox |
OmarBasem
suggested changes
Apr 25, 2023
flexsurfer
force-pushed
the
feature/fix-fn-usage-in-hiccup
branch
from
6cc76ed
to
9267431
Compare
|
@OmarBasem done |
OmarBasem
reviewed
Apr 25, 2023
Comment on lines
110
to
111
| (defn photo-selector | ||
| (defn f-photo-selector | ||
| [{:keys [scroll-enabled on-scroll]}] |
There was a problem hiding this comment.
and photo-selector please
OmarBasem
approved these changes
Apr 25, 2023
flexsurfer
force-pushed
the
feature/fix-fn-usage-in-hiccup
branch
from
9f2720f
to
2cca524
Compare
jakubgs
added a commit
that referenced
this pull request
May 8, 2023
Contains changes that should have been included in: #15713 Signed-off-by: Jakub Sokołowski <jakub@status.im>
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: #15703
also
safe-areareplaced by constants, now it can be used in any place as(safe-area/get-insets)I've seen a few places where
:f> (fn []was used two times in the same component, so I'm not sure these components will work as expected after this change cc @OmarBasem