[#15741] Fix error on tapping pinned message #15754
Conversation
Jenkins BuildsClick to see older builds (8)
|
rasom
force-pushed
the
fix/#15741-exception-on-tap
branch
from
c277411
to
9d6a1df
Compare
| @@ -97,7 +97,7 @@ | |||
| :style {:border-radius 16 | |||
| :opacity (if (and outgoing (= outgoing-status :sending)) 0.5 1)} | |||
| :on-press (fn [] | |||
| (if (and platform/ios? @keyboard-shown) | |||
There was a problem hiding this comment.
@rasom, I understand the scope is for a fix, but what's going on under the hood is more important imo. The atom should be defined in the first place, so the fix to me should be somewhere else.
I traced the argument keyboard-shown and found a place where it's not passed as an arg https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20
Unfortunately hiccup is a bit nasty, because [message/message-with-reactions message context] is a vector, there's nothing checking that the message/message-with-reactions arity is wrong. Look what I get if I change the vector to a function call, I get a warning from clj-kondo and the code won't actually compile.
invalid-arity status-im2.contexts.chat.messages.content.view/message-with-reactions is called with 2 args but expects 3 (lsp)
So I think the correct fix is to fix the bad hiccup https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20 and pass along the keyboard-shown atom. You'll need to create it and pass it as the render-data of the flat-list
These days I hardly like hiccup... so much trouble for so little, anyway.
There was a problem hiding this comment.
@ilmotta is there something bad with passing (atom false) as a third argument to https://github.com/status-im/status-mobile/blob/develop/src/status_im2/contexts/chat/menus/pinned_messages/view.cljs#L20 ?
There was a problem hiding this comment.
I think it's fine @OmarBasem, as long as we respect the function arity and don't pass an atom as undefined it's better.
There was a problem hiding this comment.
Maybe for the future, not in this PR, but the "perfect" fix could be to avoid passing keyboard-shown all the way down do other components and instead use a hook right at the place where it's needed. The hook would probably be similar or identical to the one we discussed these days here #15736 (comment)
There was a problem hiding this comment.
@ilmotta I don't think that would be a better solution. The keyboard-shown value is needed when pressing on a message. Defining the hook in the message component we would create a new hook for every message.
There was a problem hiding this comment.
Correct, hooks are cheap, but not that cheap. For long lists of things it's not a good idea, good point :)
There was a problem hiding this comment.
hey guys, changed it to (atom false), thanks
rasom
force-pushed
the
fix/#15741-exception-on-tap
branch
from
9d6a1df
to
36ffb63
Compare
rasom
force-pushed
the
fix/#15741-exception-on-tap
branch
from
36ffb63
to
fb326bb
Compare
fix #15741
status: ready