-
Notifications
You must be signed in to change notification settings - Fork 509
feat(message-expiry): Add message expiry msg on the right side bar #16690
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: main
Are you sure you want to change the base?
Conversation
| <LobbyStatus v-if="canFullModerate && hasLobbyEnabled" :token="token" /> | ||
| <div v-if="isMessageExpirationSet && !isOneToOne" class="group-message-expiration"> | ||
| <IconClockOutline :size="16" /> | ||
| <span>{{ t('spreed', 'Message expiration is set to {date}', { date: messageExpirationDate }) }} </span> |
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.
instead of calculating a random date that would need live updating, I would show something like: "Message expiration is set to 30 minutes"
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.
That is rather the actual expiration timestamp coming from conversation.lastMessage.expirationTimestamp. Should we still replace it in terms of number of minutes / hours / days?
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.
Yes, it should indicate duration
Signed-off-by: Shravan Balasubramanian <shravanayyappa@gmail.com>
5e93b1c to
074b3f4
Compare
| if (props.isMessageExpirationSet) { | ||
| fields.push({ | ||
| key: 'message-expiry', | ||
| icon: IconDeleteClockOutline, | ||
| label: t('spreed', 'Message expiration is set to {date}', { date: props.messageExpirationDate }), | ||
| }) | ||
| } |
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.
This part should be enough, the whole logic doesn't belong to RightSidebar.vue, and does not need props to be passed. We already have a conversation object here, maybe just need to be rearranged:
const profileInformation = computed(() => {
const fields = []
if (profileInfo.value?.role || profileInfo.value?.pronouns) {
...
if (profileInfo.value?.organisation || profileInfo.value?.address) {
...
if (conversation.value.messageExpiration) {
fields.push({
key: 'message-expiry',
icon: IconDeleteClockOutline,
label: t('spreed', 'Message expiration set: {duration}', { duration: expiration.label }),
})
}
return fields
}duration string can be composed from strings here
spreed/src/components/ConversationSettings/ExpirationSettings.vue
Lines 61 to 66 in e35af19
| { id: 3600, label: n('spreed', '%n hour', '%n hours', 1) }, | |
| { id: 28800, label: n('spreed', '%n hour', '%n hours', 8) }, | |
| { id: 86400, label: n('spreed', '%n day', '%n days', 1) }, | |
| { id: 604800, label: n('spreed', '%n week', '%n weeks', 1) }, | |
| { id: 2419200, label: n('spreed', '%n week', '%n weeks', 4) }, | |
| { id: 0, label: t('spreed', 'Off') }, |
'Message expiration set: {duration}' is already existing and translated string, so we e.g. can backport it to older versions right away
| this.notifyUnreadMessages(null) | ||
| // FIXME collapse for group conversations until we show anything useful there | ||
| this.contentModeIndex = this.isOneToOne ? 1 : 0 |
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.
This is the place, thanks for finding
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.
IMO although it's good to have a test coverage, it doesn't worth it in this case. Efforts to write / review tests for this exceed the implementation of the feature, that should be just a piece on top of already working code
☑️ Resolves
🖌️ UI Checklist
🖼️ Screenshots / Screencasts
🏁 Checklist