Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: archive conversations (frontend) #13358

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: archive conversations (frontend) #13358

wants to merge 2 commits into from
+152 −19

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Sep 20, 2024 •
edited
Loading

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

A B
image image
image image

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@Antreesy Antreesy added this to the 🖤 Next Major (31) milestone Sep 20, 2024
@Antreesy Antreesy self-assigned this Sep 20, 2024
@Antreesy Antreesy force-pushed the feat/noid/archive-conversations-frontend branch from 5538bee to a807cbc Compare September 20, 2024 09:40
Base automatically changed from feat/noid/archive-conversations to main September 20, 2024 09:41
@Antreesy Antreesy force-pushed the feat/noid/archive-conversations-frontend branch from a807cbc to 34392f4 Compare September 20, 2024 11:32
@Antreesy Antreesy force-pushed the feat/noid/archive-conversations-frontend branch from 34392f4 to 1e76d5f Compare September 20, 2024 11:33
@Antreesy Antreesy marked this pull request as ready for review September 20, 2024 11:34
DorraJaouad
DorraJaouad reviewed Sep 20, 2024
View reviewed changes
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality is good but a few notes about how we will implement it in LS.

src/utils/conversation.js
Comment on lines +50 to +51
|| (conversation.notificationLevel === PARTICIPANT.NOTIFY.ALWAYS && hasUnreadMessages(conversation))
|| (conversation.notificationLevel === PARTICIPANT.NOTIFY.MENTION && hasUnreadMentions(conversation))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notification level can be ALWAYS or (DEFAULT and it is 1-1 conv) for unread messages
can be ALWAYS or DEFAULT or MENTION for unread mentions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we discussed to not make it dependant on the level for now? Direct mention will show it again, regardless of notification setting ?

src/store/conversationsStore.js
@@ -904,7 +927,7 @@ const actions = {
async setNotificationLevel({ commit }, { token, notificationLevel }) {
try {
await setNotificationLevel(token, notificationLevel)
commit('setNotificationLevel', { token, notificationLevel })
commit('setNotificationLevel', { token, notificationLevel: +notificationLevel })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there an issue you noticed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we receive a number from server, and change it to string then update

src/store/conversationsStore.js
await archiveConversation(token)
}

const conversation = Object.assign({}, getters.conversations[token], { isArchived: !isArchived })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const conversation = Object.assign({}, getters.conversations[token], { isArchived: !isArchived })
const conversation = {
...getters.conversations[token],
isArchived: !isArchived
}

src/components/LeftSidebar/LeftSidebar.vue
@@ -45,6 +45,15 @@
{{ t('spreed', 'Filter unread messages') }}
</NcActionButton>

<NcActionButton close-after-click
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capability support check?

src/components/LeftSidebar/LeftSidebar.vue
Comment on lines +290 to +307
<NcButton v-if="isFiltered"
type="tertiary"
wide
@click="handleFilter(null)">
<template #icon>
<FilterRemoveIcon :size="20" />
</template>
{{ t('spreed', 'Clear filters') }}
</NcButton>
<NcButton v-else
type="tertiary"
wide
@click="handleFilter('archived')">
<template #icon>
<IconArchive :size="20" />
</template>
{{ t('spreed', 'Archived conversations') }}
</NcButton>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we need another clear filter button here.
Yet, Archived conversations should be shown ONLY if there are archived ones indeed.

I think you put filter button here so it undoes what archived conversation button do? in that case, we show return to conversations or sthg like that (we don't show to user this feature as a filter but as another page/tab in left sidebar ;) ). Also, we won't show it in filters action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SystemKeeper SystemKeeper SystemKeeper left review comments

@DorraJaouad DorraJaouad DorraJaouad left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Antreesy Antreesy

Labels
3. to review enhancement feature: conversations 👥 feature: frontend 🖌️ "Web UI" client
Projects
None yet
Milestone
🖤 Next Major (31)
Development

Successfully merging this pull request may close these issues.
3 participants
@Antreesy @SystemKeeper @DorraJaouad