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.

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

feat(integration): show out-of-office message in 1-1 conversation #10902

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Nov 14, 2023

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Main chat:

Screencast.from.29.11.2023.11.35.50.webm

Sidebar chat:
image

🚧 Tasks

  • Move to pinia store
    • Add test coverage
  • Handle errors from server if any

🏁 Checklist

@Antreesy Antreesy added 2. developing enhancement feature: frontend 🖌️ "Web UI" client feature: integration 📦 Integration with 3rd party (chat) service labels Nov 14, 2023
@Antreesy Antreesy added this to the 💙 Next Beta (28) milestone Nov 14, 2023
@Antreesy Antreesy self-assigned this Nov 14, 2023
@Antreesy Antreesy force-pushed the feat/10599/show-out-of-office-message branch 2 times, most recently from 612063a to 6e94192 Compare November 15, 2023 12:56
@marcoambrosini
Copy link
Member

@nextcloud/designers should we shrink the width to match the quote component?

@Antreesy
Copy link
Contributor Author

should we shrink the width to match the quote component?

Looks good as well. Also won't overlap with 'Scroll to bottom anymore, so it's one problem less:
image
image
image

Some styles should be adjusted, though, because we don't have to align it with messages anymore

src/components/TopBar/TopBar.vue Outdated Show resolved Hide resolved
src/stores/absence.js Outdated Show resolved Hide resolved
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.

I didn't mean to approve 🙈

@nimishavijay
Copy link
Member

Looks really nice when the width is adjusted! :) 2 small questions/feedback:

  • What do you think about making both the widths equal to the input field? Was there already a discussion about this during the quote reply feature? It would look something like this (and the OOO notecard can also be the same width):
    image

  • Instead of the avatar, we could use an icon indicating unavailability as Google does (source: this Zapier post). Maybe this event-busy MDI icon? What do you think? :)

@Antreesy
Copy link
Contributor Author

What do you think about making both the widths equal to the input field? Was there already a discussion about this during the quote reply feature?

Was like this since the beginning: #4584, but I can't see the reasons to align the widths.

Instead of the avatar, we could use an icon indicating unavailability

Also agreed, as we don't have to align it with messages list anymore.

@marcoambrosini
Copy link
Member

@nimishavijay the avatar is there because there is a message from the person just below (See the first screenshot). This whole message should be expanded by default.

To make this clearer when it's collapsed, we could use a layout similar to the NcListItem component. In the first line we could have the warning, and in the second line a preview of the message.


About the with of both component. Could we create a separate issue and see before and after screenshots? I think I tried that back then and it looked worse.

@Antreesy
Copy link
Contributor Author

Not sure if we really need an avatar here (it takes additional time to load render, and consumes 48px of space on small screen)
I'd suggest at least use the same size as on replies

44px 22px
image image

Also checked different widths, and full one doesn't look good above pill-shaped input IMO

Shrink Full-width
Screenshot from 2023-11-29 10-53-43 Screenshot from 2023-11-29 10-54-33

@Antreesy Antreesy force-pushed the feat/10599/show-out-of-office-message branch from bd9d73d to 6da90ad Compare November 29, 2023 10:42
@Antreesy Antreesy marked this pull request as ready for review November 29, 2023 10:44
@Antreesy Antreesy force-pushed the feat/10599/show-out-of-office-message branch from 6da90ad to 94d5772 Compare November 29, 2023 10:45
Comment on lines 405 to 406
if (!conversation?.status || conversation.status !== 'dnd'
|| !this.isOneToOneConversation || this.chatExtrasStore.absence[this.token] !== undefined) {
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
if (!conversation?.status || conversation.status !== 'dnd'
|| !this.isOneToOneConversation || this.chatExtrasStore.absence[this.token] !== undefined) {
if (!conversation?.status || conversation.status !== 'dnd'
|| !this.isOneToOneConversation ) {

It is already being checked inside the store
Also, conversation.status !== 'dnd' is also questionable because the status update wasn't updated in some tests while the OoO is there. I am not sure how it is synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking here specifically for undefined. If user has standard DnD status, but without "Out of office" it will send a request to server each time you open the tab, and receive null. With that line, if stored value has null, request will be sent only once.

Once status message_id will be exposed (should be 'vacationing'), we can switch to checking it, and refactor this code
cc @nickvergessen should i raise an issue to track it? server or spreed repo?

Copy link
Member

Choose a reason for hiding this comment

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

server, but it's unlikely to happen at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

In the last 3 attempts , the status wasn't updated.
image
image

src/stores/chatExtras.js Outdated Show resolved Hide resolved
@@ -54,6 +54,10 @@

<!-- Input area -->
<div class="new-message-form__input">
<NewMessageAbsenceInfo v-if="userAbsence"
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 should also check the dates. For example, I can set my OoO proactively from tomorrow and I am still working today. AbsenceInfo should be shown starting from tomorrow and not from now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then OoO should be set up from tommorow by BG job, I don't think it has to be double-checked on Talk side

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't sure if it should be from the server side to only render the active OoO or any OoO that exists because dates were given in the response.

Should we use them to show when they will be back for example ? like on Webex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the next iteration maybe

@import '../../assets/variables';

.absence-reminder {
margin: 0 calc(var(--default-clickable-area) / 2) 12px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree with the full width as a non-optimal form and shorter width is better. However, I recommend setting the width right at the end of the curve of the input pill shape.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be margin 16px I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but pill border-radius is already calc(var(--default-clickable-area) / 2). So, technically, 22px is correct (but maybe visually 16px would be better)

@nickvergessen
Copy link
Member

Btw the short status message is not used anywhere. I guess it should be the text on the actual user status (so upstream issue) or would it appear in the notecard?

@DorraJaouad
Copy link
Contributor

Btw the short status message is not used anywhere. I guess it should be the text on the actual user status (so upstream issue) or would it appear in the notecard?

It is not shown anymore in the notecard, only long description is used but we can use the short description as a fallback if long description is empty.

If both are empty, the arrow button should be removed.
image

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.

Full width looks good to me, so no strong opinions from my side on that

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.

It is ready 🦅

@nickvergessen
Copy link
Member

Time to fixup then

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feat/10599/show-out-of-office-message branch from ab8b77e to 54655bf Compare November 30, 2023 10:02
@Antreesy
Copy link
Contributor Author

/backport to stable28

@Antreesy Antreesy merged commit 40b002d into main Nov 30, 2023
36 checks passed
@Antreesy Antreesy deleted the feat/10599/show-out-of-office-message branch November 30, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing enhancement feature: frontend 🖌️ "Web UI" client feature: integration 📦 Integration with 3rd party (chat) service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show out-of-office message in 1:1 conversations
5 participants