-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
Select multiple messages in chat for forwarding & deletion #3606
Conversation
BUTTON BUTTON BUTTON NEW NEW NEW BUTTON!
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 can see some nice patterns here with dedicated components and reducer logic for the selection logic.
As written above I'd suggest to factor out the state as well into an own context, to avoid prop drilling and making Message
and MessageList
more complex.
You already have some code isolation in place and we could go even further. I'd suggest to create a new folder and move a context implementation, your new components, the reducer and maybe even the CSS module there to make it all represent the "multi select" feature, completly independent from the rest.
scss/manifest.scss
Outdated
@import 'message/_selected-messages-action.scss'; | ||
@import 'message/_select-mode-mask.scss'; |
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.
It would be nice to use CSS modules here instead if possible
selectMessage: (id: number) => void | ||
unselectMessage: (id: number) => void | ||
selectedMessages: number[] |
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.
MessageList
and Message
get quite many new props here which I wouldn't see directly as part of the message component itself.
Maybe we can introduce a context here instead to manage the selection state for messages? The context lives in the currently displayed chat and every message can consume the state of the context to understand if it is selected or not.
I've seen you already have a nice reducer. With the context representing the state and the reducer changing it we can have a nice isolated logic. All of this can be moved into own files.
Let me know if you need any help with my suggested changes! Also I've changed the target branch to the button refactor one as this branch includes these changes as well, hope that's okay. |
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.
does not work for me or I don't understand how to do it, lets put it into draft state again until it is mergeable.
I've began to use Contexts thanks to adz. You can omit my last commit to see the working version with props drilling. |
On second thought, please give me some more days. |
Okay I'm stuck, man! |
Different adz I think. Interesting project though. |
84399b0
to
b357d4b
Compare
This is a really nice feature and I'd love to see it getting released, maybe in |
30c3ce2
to
99949c3
Compare
Discussed with nico and farooq -- putting this into project resurrection for when the time comes! |
This PR aims to implement selecting multiple messages for forwarding and deleting. Reviews are welcome after christmas :)
this would solve one part of #2892 (that issue is about multi select in chatlist, message list and gallery)