Skip to content

Conversation

@PVince81
Copy link
Member

@PVince81 PVince81 commented Apr 29, 2021

Fixes #3110

When the sidebar is closed while in call, there will be an unread messages counter on top of the "open sidebar" button:
image

It is blue for new mentions and grey if there are no mentions.

Additionally, the first time a new unread message arrives or a new mention, a notification will be displayed.

@PVince81
Copy link
Member Author

@ma12-co I'm unsure about the bubble alignment. It needs to be positioned in a way that it still looks nice when there are more digits.

Also: I had to reuse the "AppNavigationCounter" outside the app navigation because somehow we didn't make that a generic numbers bubble component ?

@PVince81
Copy link
Member Author

PR for CounterBubble: nextcloud-libraries/nextcloud-vue#1895

@nickvergessen nickvergessen marked this pull request as draft April 30, 2021 11:55
@nickvergessen
Copy link
Member

Requirement is full filled, so this can be finished?

@PVince81
Copy link
Member Author

Requirement is full filled, so this can be finished?

oh, yes, finally!!

@PVince81 PVince81 force-pushed the enh/3110/in-call-new-messages-indicator branch from 18dd122 to 2af8a2e Compare June 14, 2021 09:40
@PVince81
Copy link
Member Author

rebased and retested, seems to work.

however can't test for too long due to the unrelated scrolling regression: #5747

@PVince81 PVince81 marked this pull request as ready for review June 14, 2021 09:41
@PVince81 PVince81 requested a review from nickvergessen June 14, 2021 09:42
@marcoambrosini
Copy link
Member

marcoambrosini commented Jun 14, 2021

@PVince81 could we use a chat icon as default icon while in call, instead of the sidebar-participants hybrid one?

like mdiMessageText
message-text

@PVince81
Copy link
Member Author

@PVince81 could we use a chat icon as default icon while in call, instead of the sidebar-participants hybrid one?

like mdiMessageText
message-text

I've just tried this locally and it seems the Actions component is very glitchy.

When starting the call the icon doesn't appear even though the Vue inspector tells me the MessageTextIcon is there.
I have to open and then close the sidebar again to make it appear.

And then when I leave the call, the icon is still there even though isInCall should be false. Something weird going on... I'll try to debug this, maybe the reactivity has some trouble.

@PVince81
Copy link
Member Author

what the hell, ghost components...
image

@PVince81
Copy link
Member Author

I wonder if it's a bug in the vue transition and that it mistakenly fails to remove the DOM element even though the component is not there any more. But I haven't found where the right sidebar transition is defined to disable it for testing.

Worst case I replace the material design icon with a regular one...

@PVince81
Copy link
Member Author

and here the glitch where the Actions render the single ActionButton hidden the first time:
image

image

@PVince81
Copy link
Member Author

alright, I managed to get rid of the glitches by setting "key" attributes on the buttons... I'll do more testing then push

@PVince81 PVince81 force-pushed the enh/3110/in-call-new-messages-indicator branch from 2af8a2e to e3113d8 Compare June 14, 2021 15:18
@PVince81
Copy link
Member Author

rebased and adjusted, changes:

  • Replace icon with MessageText when in call.
  • Counter bubble now lets hover and click through.
  • Display two notifications when mentioned.

@nickvergessen
Copy link
Member

When the first message is a mention I get this:

Bildschirmfoto von 2021-06-17 16-00-12

Otherwise looks like a solid idea

@PVince81
Copy link
Member Author

PVince81 commented Jun 17, 2021

When the first message is a mention I get this:

Bildschirmfoto von 2021-06-17 16-00-12

I thought this is what you wanted when you commented on the line that had "unreadMessages > 1" instead of "> 0".
Sadly I force pushed back then so can't re-read it.

If that's a concern I can make the messages to be a single one again, with "you have been mentionned" with higher priority.

@nickvergessen
Copy link
Member

What I ment was:

  1. someone writes a message:

You have new unread messages in the chat

  1. (You still didn't check the chat) someone mentions you:

You have been mentioned in the chat

What I didn't mean:

  1. You close the chat
  2. Someone writes a single message @admin
  3. You see 2 notifications for 1 message

@PVince81
Copy link
Member Author

now thinking, I could simply store the notification handle and hide it to make sure that we only ever see one notification.

so in the first scenario you'd only see "you have been mentioned" at the end.

@nickvergessen
Copy link
Member

so in the first scenario you'd only see "you have been mentioned" at the end.

Which is fine, but when there were like 5 mins between the normal message and a mention its good to reping I guess. That's why I ment that 2 notifications are prolly fine

@PVince81
Copy link
Member Author

so in the first scenario you'd only see "you have been mentioned" at the end.

Which is fine, but when there were like 5 mins between the normal message and a mention its good to reping I guess. That's why I ment that 2 notifications are prolly fine

Yes. I've just pushed after testing and this is how it behaves now.

@PVince81
Copy link
Member Author

unit test added for the messages store adjustments.

TopBar component tests have not been bootstrapped yet, maybe in a separate PR

@PVince81 PVince81 marked this pull request as ready for review June 17, 2021 15:29
@PVince81 PVince81 requested a review from nickvergessen June 17, 2021 15:29
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I slightly modified the position of the icon and the counter.

One thing I'm not convinced is the toast for new messages. I think it's really invasive, especialle because it covers buttons making them unclickable. I'd much rather leave it as is or add a subtle animation to the counter each time the number increases. What do you think?

PVince81 and others added 8 commits June 17, 2021 20:47
When in a call and the sidebar is collapsed, whenever new unread
messages arrive, a bubble will be displayed on top of the button that
opens the sidebar.

If a mention exists, the bubble will appear highlighted.

Whenever a new unread message or new mention arrives while in a call
with the sidebar closed, a notification will appear once.

Signed-off-by: Vincent Petry <[email protected]>
Co-authored-by: Joas Schilling <[email protected]>
Replace icon with MessageText when in call.
Counter bubble now lets hover and click through.
Display two notifications when mentionned.

Signed-off-by: Vincent Petry <[email protected]>
As soon as we receive new messages from the "lookForNewMessages"
action, they are counted and added to the number of known unread
messages.

This makes the store update and counter in the left sidebar update more
quickly.

Signed-off-by: Vincent Petry <[email protected]>
When fetching new messages in the current conversation, also scan them
for mentions to update the conversation store's unreadMention flag if
necessary.

This makes a mention notification or the counter bubble highlight
color update more quickly without having to wait for the conversation
list reload.

Signed-off-by: Vincent Petry <[email protected]>
Adjust icon size.
Prevent double notification.

Signed-off-by: Vincent Petry <[email protected]>
Only show one notification for unread messages, either for new messages
or for mentions.
When switching from "new messages" to "you have been mentionned",
discard the former notification.

Signed-off-by: Vincent Petry <[email protected]>
Minor fixes to messagesStore unread messages counter logic.
Added tests for all cases.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 force-pushed the enh/3110/in-call-new-messages-indicator branch from f83268d to df04143 Compare June 17, 2021 18:48
@PVince81
Copy link
Member Author

One thing I'm not convinced is the toast for new messages.

This is a general problem, also when having connection issues or device issues.
Ideal would be to be able to define an offset for the toast container, or is it possible to specify a different container perhaps ?

To fix the messed up rebase, I force pushed my local work and then cherry-picked your last commit, then rebased again.
The commit history should be fine now.

@PVince81
Copy link
Member Author

for the toast notification, I found the "selector" attribute, but setting it to "#call-container" didn't shift it.

@PVince81
Copy link
Member Author

@marcoambrosini I managed to shift the notification down, see #5794

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Nice and neat addition

@nickvergessen nickvergessen merged commit 95c25f4 into master Jun 18, 2021
@nickvergessen nickvergessen deleted the enh/3110/in-call-new-messages-indicator branch June 18, 2021 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new message indicator when sidebar is collapsed

5 participants