Fixed show user pop up system#1541
Conversation
Moved all the added features in the "ShowUserPopUp_System" in here now by the looks of it works same as the originally had (please ignore the mess from other commits)
| _longClick = true; | ||
|
|
||
| _reactionPaneldata[0]._reactionField.transform.SetParent(_chatShowUsersPopUpData._reactionFieldNewLocation.transform); | ||
| _usersWhoAdded.transform.SetParent(Chat.instance.PopUps.transform); |
There was a problem hiding this comment.
Miksi tämän parenttia siirretään??? Eikö olisi helpompi että olisi yksi popup jota vain päivitetään sitä mukaan kun sitä tarvitaan.
|
|
||
| _longClick = true; | ||
|
|
||
| _reactionPaneldata[0]._reactionField.transform.SetParent(_chatShowUsersPopUpData._reactionFieldNewLocation.transform); |
There was a problem hiding this comment.
Miksi näitä reaktio bokseja siirrellään paikasta toiseen? Josset halua luoda kokonaan uusia bokseja ja asettaa niiden tietoja, niin ainakin voisi olla fiksumpaa kopioida se olemassa oleva kuin siirtää niitä. Tämä aiheuttaa selkeästi sitä, että boksit ovat hetkellisesti väärässä paikkaa kun popupikkuna suljetaan.
|
|
||
| public MessageObjectHandler _messageObjectHandler; | ||
| [SerializeField] private ChatShowUsersPopUpData _chatShowUsersPopUpData; | ||
| public LayoutElement layoutElement; |
There was a problem hiding this comment.
Onko mitään syytä miksi tämä on public? Eikös tämä voisi olla private SerializeField tagillä.
| using Altzone.Scripts.Chat; | ||
| using UnityEngine; | ||
| using UnityEngine.UI; | ||
| using static System.Collections.Specialized.BitVector32; |
There was a problem hiding this comment.
Turha roska, jonka koodieditori on lisännyt. Poista.
| public RectTransform _baseMessageSize; | ||
| [SerializeField] private ChatMessageScript backgroundSize; | ||
| [SerializeField] private GameObject reactionField; | ||
| public Vector2 reactionFieldVector; |
There was a problem hiding this comment.
Miksi tämä tai nuo pari seuraavaa fieldiä ovat julkisia? Nämähän voisivat olla yksityisiä.
| using Altzone.Scripts.Model.Poco.Player; | ||
| using static ServerChatMessage; | ||
| using System.Linq; | ||
| using static ChatShowUsersPopUpData; |
There was a problem hiding this comment.
Tämähän on täysin turha koska se kutsuu itsensä.
| { | ||
| _closeAllReactionButton.onClick.AddListener(LayoutButton); | ||
| _listOrderButton.onClick.AddListener(() => currentOrder++); | ||
| _listOrderButton.onClick.AddListener(() => ListOrder(currentOrder)); |
There was a problem hiding this comment.
Eikös tämä ja rivi 52 voisi olla saman listenerin sisällä?
| _messageReactionsHandler.ReactionResize(); | ||
|
|
||
| RectTransform rt = _messageObjectHandler.ReactionsPanel.GetComponent<RectTransform>(); | ||
| gameObject.transform.SetParent(ShowUsersPopUp.transform); |
There was a problem hiding this comment.
Sama mistä tein jo kommentin, mutta nyt vain toisinpäin, tarvitaanko jokaiselle viestille oma popup? Eikö yksi riittäisi, jolloin sitä ei tarvitsisi siirrellä koko ajan kun sitä piilotetaan.
| public class ChatReactionHandler : MonoBehaviour | ||
| { | ||
| [SerializeField] private GameObject _messageReaction; | ||
| public GameObject _messageReaction; |
There was a problem hiding this comment.
Eikös tähän olisi parempi tehdä Getteri, jos tätä tarvitsee lukea ulkopuolelta.
Silloin ei vahingossa avata mahdollisuutta muuttaa tuota objektia itsessään.
|
|
||
| private void Start() | ||
| { | ||
| instance = this; |
There was a problem hiding this comment.
En tekisi tähän Instance rakennetta, varsinkaan tähän tarkoitukseen mikä tässä on kun tämän voisi tehdä helposti myös toisinpäin että tälle skriptille kerrotaan mitä halutaan tehdä ja tämä tekee sen sen sijaan että lapsi objektista turhaan haetaan tämän tietoja.
BillTheBeast
left a comment
BillTheBeast
left a comment
There was a problem hiding this comment.
Tuo että onko jokaisella viestillä oma rektio popup vai onko siellä vain yksi joka päivitetään tarpeen mukaan, on hyvä kysymys, mutta pitääkin kysyä että mitkä ovat näiden toteutusten hyvät puolet ja huonot puolet.
Kun jokaisessa viestissä on oma popup:
- Tietojen päivittämisen pystyy suorittamaan reaaliajassa, jolloin popupin avaaminen pitäisi olla nopeampaa kun ei tarvitse tietoja siirtää samalla.
- Mutta tämä tarkoittaa että jokaisessa ikkunassa on "ylimääräinen" komponentti, joka pitää ladata muistiin.
- Myös chat ikkunan avaaminen on hieman hitaampaa kun nämä popupit pitää päivittää tässä kohtaa.
- Lisäksi tämän rakenteen hallinnoiminen on huomattavasti hankalampaa ja helposti aiheuttaa ongelmia, jos tuon toimintaa pitää mennä muokkaamaan myöhemmin, koska se sisältää noita popupin uudelleen sijoitteluita.
Itselleni tuo viimeinen olisi se isoin ongelma, varsinkin kun tässä ihmiset vaihtuu vähän väliä niin pitäisi selittää miten se järjestelmä toimii. Ja sitä yhden popupin toteutustakin voi helposti optimisoida jälkikäteen, jolloin tuo päällimmäinen hyöty tästä useamman popupin rakenteesta katoaa jonkin verran.
Changed ShowUserPopUp so taht now its on ChatView instead of Basemessages. there's still issues with toggle system to _usercontent and reaction button on popup do not work right now
2 participants
Chat feature where user can see who has send reaction on set message + now is on updated main
does not have Users Avatar (by the reactions sender so it uses the message senders Avatar instead)