Repository navigation
Conversation
- change signature of ChatMessage to allow a content list instead of just a text - this enables the component to embed nested components and multiple texts - change signature of ChatDisplayModel: getChatMessages instead of getPreviousMessages must be implemented - add initialTopMessageId to the component - add command scrollToMessage to the component - add event messageViewed to the component - add event photoClicked to the component - using photo thumbnail if given - remove workaround "scroll to bottom" on resize
- add message footer - add message colors (border, bg, text) - enhance photo gallery rendering - stabilized scrolling position - lazy loading images
…to its previous handling of photos
|
This is two merge requests in one... I'll review it but it would be better if there was one for the new component (MasonryGalery) and one for the changes to the ChatDisplay. |
yamass
left a comment
yamass
left a comment
There was a problem hiding this comment.
I did not do a full review. At this point, my remarks are worth a deep discussion about the requirements and implementation. Please, let's have a chat.
| * @param numberOfOlderMessages maximum number of messages to include before the active lower boundary | ||
| * @return message batch in ascending chronological order | ||
| */ | ||
| ChatMessageBatch getChatMessages(Integer exclusiveStartingMessageId, Integer inclusiveAnchorMessageId, int numberOfOlderMessages); |
There was a problem hiding this comment.
issue: I find quite everything about this method weird. It feels like a Frankenstein of the three methods above it. But that makes it more complicated than things need to be.
Also, this is not really an interface method. Nobody outside this interface needs it. It is dictating how to do the implementation for no reason.
Suggestion:
- Drop this method again
- Make
getLastChatMessages(int numberOfMessages)delegate togetPreviousMessages(Integer earliestKnownMessageId, int numberOfMessages)again (as before) getLastChatMessages(int oldestMessageId, int bufferNumberOfMessages): Rename method and parameters to:getMessagesStartingFrom(int messageId, int lookBehind)
| return new ChatMessageTextContent(text); | ||
| } | ||
|
|
||
| static ChatMessageContent component(Component component) { |
There was a problem hiding this comment.
issue: I find it quite strange to ask the model of a component to come up with the actual component to use for rendering its data. It is called model view controller for a reason. The model should not come up with the view.
How about:
- getting rid of this class
- introducing a
getMetadata()method to theChatMessageclass. Something likeMap<String, Object>. If you really insist, you can make your model put a component in there, but the cleaner way would be to only put the actual data to display there. - When the
ChatMessagegets translated to aUiChatMessage, there must be a hook for translating metadata to whatever you want to do/add to the chat message. This should be a functional interface as always.
|
|
||
| String getText(); | ||
|
|
||
| default List<ChatMessageContent> getContent() { |
There was a problem hiding this comment.
issue: This is weird. We now have getText() and getContent() which in turn has getText() and getComponent().
Why not just getText() and getComponent().
Why a list?
But anyway, I am against introducing components to the model anyway. See my comment in ChatMessageContent.
| String userImageUrl; | ||
| String userNickname; | ||
| String text; | ||
| List<UiChatMessageContent> content; |
There was a problem hiding this comment.
issue: Again: Why not just component. Why the additional wrapper?
| return null; | ||
| } | ||
|
|
||
| default String getFooter() { |
There was a problem hiding this comment.
What's a footer? If you need to display the time of writing, why not add that? Or is there a larger requirement behind this?
As a footer this will look quite lost. Can be made much more beautiful as a dedicated time element.
2 participants
Change ChatDisplay component:
Created new MasonryGallery component to enhance visualization of photos for chat messages (or other use-cases)