Add Chat-Bubbles Feature#29
Conversation
paulikauro
left a comment
There was a problem hiding this comment.
Cool! One general comment: "Chat-Bubble" -> "chat bubble". Some simplifications and a few things to think about in code comments
paulikauro
left a comment
There was a problem hiding this comment.
Nice work! There are some ACF things that would be good to address at this point:
- commands should take
target: OnlinePlayer Bubbleshould get its own ACF context (like mentioned previously)- additionally, the
Bubblecontext should have a flag for checking bubble ownership. Then you can simply do@Flag(FLAG_BUBBLE_OWNED) bubble: Bubblewhenever a command needs the sender to be an owner of a bubble (delete, kick, setPrivate). (ThereFLAG_BUBBLE_OWNEDis just a constant string; compare to egUserCache.COMPLETION_USERNAMES, except this one doesn't have to be public)
Once implemented, these will make the commands shorter and nicer to read, plus there'll be less duplication :)
|
Review requested! |
paulikauro
left a comment
There was a problem hiding this comment.
Nice, some small details still and a few bigger suggestions
`sender` is never Player, so the else branch was always taken. Now every error will also get the info prefix added to it. In the future, a sendError may be useful to add.
|
I'd like to formally request a review from the great PaukkuPalikka again |
|
I'd like to formally request a review from the great PaukkuPalikka or the great Nickster again |
paulikauro
left a comment
There was a problem hiding this comment.
Looks good! A couple minor things in the comments. I think the only remaining thing we should address is shout going thru the chat filter
|
I forgot shout already did its thing. So the thought here would be to reserve |
- bubble list only shows bubbles you can access, unless you have the manage permission - some commands (join) now let you join if you can't access the bubble if you have the manage permission
Append causes some styles to get inherited where they aren't welcome. textOfChildren is the right way to concatenate components.
This PR adds what the title says 🧋