-
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
Add popup menu support for chat header and chat list #6671
Conversation
Pull Request Checklist
|
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.
Nice PR! Included a couple questions inline.
(def popup-menu-options (adapt-class (.-MenuOptions js-dependencies/popup-menu))) | ||
(def popup-menu-option (adapt-class (.-MenuOption js-dependencies/popup-menu))) | ||
(def popup-menu-trigger (adapt-class (.-MenuTrigger js-dependencies/popup-menu))) | ||
(def popup-menu-renderers (js->clj (.-renderers js-dependencies/popup-menu) :keywordize-keys true)) |
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'm lacking context here clearly, but what is the guideline for converting to clj data structure in the last form versus adapt-class
in the others? Thank you!
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.
renderers
is not a class, but a folder inside react-native-popup-menu
. In this case (.-renderers js-dependencies/popup-menu)
returns a js object with string keys (map of class names to objects) So we convert it to a Clojure-style keyword map.
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.
great, thanks for clarifying.
{:keys [group-chat public? public-key] :as chat}]] | ||
[popup-menu/wrap-with-menu | ||
[chat-list-item-inner-view (assoc chat :chat-id chat-id)] | ||
(remove nil? |
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.
which set of chats are expected to not be removed here? just private, or are we up to additional categories? 👁️
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.
remove
affects menu items for this particular chat item, not the chat list. So for public/group chats we cannot execute View Profile
, for example. Will write the doc for wrap-with-menu
, sorry for confusion.
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.
ah, no that makes total sense. my own confusion for missing at what level the remove
was acting upon. thanks!
@siphiuel can it be tested? If yes, can you please rebase it and move to |
6420acf
to
1b6d18b
Compare
Rebased and moved to |
a473851
to
65a7813
Compare
@SlizkovaGanna |
Tested on MacOS and Linux (emulated) Issues found: 1. The 'Unknown' user it triggered when selecting 'View profile' on right-click from the chat list, reproduces for all contacts every time
2. When triggering the right-click menu in the chat list, the menu contains options for the chat that the mouse pointer is at, but performs actions for the chat that is selected at the moment Steps to reproduce:
Same for a public chat: Also tested:
|
4bd6a28
to
8edfd42
Compare
@rcullito @Maxris builds are failing in Jenkins, i restarted it not sure about success, please take a look https://ci.status.im/job/status-react/job/pull%20requests/view/change-requests/job/PR-6671/ |
@siphiuel please rebase :) |
8edfd42
to
90c3a5b
Compare
8e8c91d
to
faa733d
Compare
97% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (57)Click to expand |
Tested on MacOS and Linux (emulated) There were no specific PR-related issues found, all the general ones found will be reported separately. @siphiuel if its possible to fix, I'll create a separate low priority issue for it. Tested:
@Serhy Also checked scenarios for 'Unknown' issue from #6210
Automated tests fails are expected. |
faa733d
to
e7ff692
Compare
Signed-off-by: Vitaliy Vlasov <siphiuel@gmail.com>
e7ff692
to
dc4841f
Compare
Fixes #5311 #5314