-
Notifications
You must be signed in to change notification settings - Fork 82
feat(jumpTo): implement lastMessage prop of chats and put add in JumpTo #18060
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
base: master
Are you sure you want to change the base?
Conversation
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 had to create this file and update all the other ones because I now import message
in the chat
DTO file and it created a recursive import, because the link preview also imports community, which imports chat, which imports message, which imports link preview 💀
Using a Utils file like this avoids the issue and is just cleaner anyway.
bottomRowComponentFillsWidth: true | ||
bottomRowComponent: Loader { | ||
active: root.lastMessageText && root.lastMessageText.length > 0 | ||
clip: true // You can't elide RichText so we use clip instead |
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.
Not sure if we can do something about that. It's not as pretty
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.
We could just use a plain text here... and display sth like [Image]
or [Sticker]
in case the message is not text
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.
Anyway, you could try converting the HTML to plain text and elide instead
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.
We have a couple options:
- Use the raw
text
property from the MessageDto- Pro: It's the simplest and elides
- Con: It contains the
*
and other Markdown elements, which are ugly
- Use the raw `text but remove the markdown elements
- Pro: would elide
- Con: I don't think we have a function to remove markdown stuff yet
- Use the
parsedText
property from the MessageDto- Pro: Also quite easy and does show what the user would see normally
- Con: Doesn't elide
- Use
parsedText
and remove the HTML- Pro: elides
- Con: has the most parsing (though not enough to cause issues I think)
@benjthayer the design doesn't have message markdown. What do you reckon would be the best? Do we want to show the rich text? Do we prefer eliding?
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 think we want to show the plain text there, so I'd go with the easiest solution that will allow us to elide using some Text
element
Jenkins BuildsClick to see older builds (13)
|
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 work! Just some minor details
@@ -71,9 +72,8 @@ type ChatDto* = object | |||
readMessagesAtClockValue*: int64 | |||
unviewedMessagesCount*: int | |||
unviewedMentionsCount*: int | |||
#lastMessage*: Message ???? It's a question why do we need it here within this context ???? |
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.
Now we know 😆
if chat.communityChannelUuid() != self.channelUuid: | ||
continue | ||
|
||
debug "setChannelInfo", communityId = $self.community.getCommunityId(), channelUuid = $self.channelUuid |
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.
leftover?
@@ -42,5 +44,16 @@ ShellGridItem { | |||
} | |||
} | |||
|
|||
// TODO bottomRowComponent -> last message in this chat | |||
bottomRowComponentFillsWidth: true | |||
bottomRowComponent: Loader { |
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.
A Loader inside a Component inside a Loader :)
bottomRowComponentFillsWidth: true | ||
bottomRowComponent: Loader { | ||
active: root.lastMessageText && root.lastMessageText.length > 0 | ||
clip: true // You can't elide RichText so we use clip instead |
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.
Anyway, you could try converting the HTML to plain text and elide instead
@@ -25,6 +25,7 @@ AbstractButton { | |||
property bool hasNotification | |||
property int notificationsCount | |||
property alias bottomRowComponent: bottomRowLoader.sourceComponent | |||
property bool bottomRowComponentFillsWidth: false |
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.
Not needed imo
@@ -42,5 +44,16 @@ ShellGridItem { | |||
} | |||
} | |||
|
|||
// TODO bottomRowComponent -> last message in this chat | |||
bottomRowComponentFillsWidth: 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.
You can use sth like:
width: root.availableWidth
on the bottomRowComponent
; see e.g. the ShellGridWalletItem
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.
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 right yeah... I'll be doing some additional tweaks to the sizes of those grid items, so you can keep it as-is for now
ae50745
to
3f37f24
Compare
cd6b871
to
a381b4f
Compare
a381b4f
to
96fb531
Compare
3f37f24
to
ded14fc
Compare
96fb531
to
f73e740
Compare
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!
Fixes #18031 Adds the `lastMessage` property to ChatDto and to the Chat Model and Item. Adds the `lastMessage` component to the JumpTo Screen Updates the `lastMessage` in the Chat model when new messages are received
f73e740
to
98149ad
Compare
What does the PR do
Fixes #18031
Adds the
lastMessage
property to ChatDto and to the Chat Model and ItemAdds the
lastMessage
component to the JumpTo ScreenUpdates the
lastMessage
in the Chat model when new messages are receivedAffected areas
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality
Storybook:

Live:
lastMessage.webm
(yeah images don't seem to work, I'll fix 😄 )
Impact on end user
Shows the lastMessage of a chat in the JumpTo screen
How to test
ENV_SHELL_ENABLED=1
Risk
Tick one: