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

techdebt(NcAvatar) - optimize avatar wrappers #10602

Merged
merged 6 commits into from Sep 29, 2023
Merged

techdebt(NcAvatar) - optimize avatar wrappers #10602

merged 6 commits into from Sep 29, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Sep 27, 2023 •
edited

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
image image
image image
image Screenshot from 2023-09-27 17-20-59

🚧 Tasks

  • Change avatar sizes in call (MediaSettings, VideoVue, LocalVue)
  • Get rid of avatar-mixin in favor of CSS variables

🏁 Checklist

@Antreesy Antreesy added this to the 💙 Next Major (28) milestone Sep 27, 2023
@Antreesy Antreesy self-assigned this Sep 27, 2023
@Antreesy
partially revert a0f8a99, enable mixin for 1-1 conversations only
450a927 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/10282/update-avatar-wrappers branch from c7a1722 to cf9d28d Compare September 28, 2023 19:29
@Antreesy Antreesy marked this pull request as ready for review September 28, 2023 19:31
DorraJaouad
DorraJaouad requested changes Sep 29, 2023
View reviewed changes
src/components/TopBar/TopBar.vue
@@ -164,7 +164,7 @@ import TopBarMediaControls from './TopBarMediaControls.vue'
import TopBarMenu from './TopBarMenu.vue'

import { CONVERSATION } from '../../constants.js'
import isInLobby from '../../mixins/isInLobby.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

isModeratorOrUser is still used from this mixin

Copy link
Contributor Author

@Antreesy Antreesy Sep 29, 2023 •
edited

Choose a reason for hiding this comment

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

Included in getParticipants

spreed/src/mixins/getParticipants.js

Line 32 in 7ced2bc

mixins: [isInLobby],

src/components/MessagesList/MessagesGroup/MessagesGroup.spec.js Show resolved Hide resolved
@Antreesy
replace single-used AuthorAvatar component with AvatarWrapper
cc6a7c5 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
show user online status in participant suggestions
ec3ac59 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
explicitly set the size for ConversationIcon
15c6bcc 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
rewrite default props for ConversationIcon.vue
ba60d64 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
[stable28] rewrite default props for GuestWelcomeWindow.vue
2111c38 
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/10282/update-avatar-wrappers branch from cf9d28d to 2111c38 Compare September 29, 2023 08:57
@Antreesy
Copy link
Contributor Author

/backport 450a927,cc6a7c564e35b8844bdaa94c2c9fcf96bf95b4dd,ec3ac59f783a9e28404d76036d203f2d1a2eac1a,15c6bcc281aa0544191af78920233f1e192cd6c1,ba60d64b4acab560a18dfd08b43d8b1c99fd9d80 to stable27

DorraJaouad
DorraJaouad approved these changes Sep 29, 2023
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.

Nothing is blocking, tested 🦅

@Antreesy Antreesy merged commit b348db3 into master Sep 29, 2023
25 checks passed
@Antreesy Antreesy deleted the fix/10282/update-avatar-wrappers branch September 29, 2023 10:12
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Sep 29, 2023
Merged
nickvergessen
nickvergessen reviewed Sep 29, 2023
View reviewed changes
src/components/MessagesList/MessagesGroup/MessagesGroup.vue
disableMenu() {
// disable the menu if accessing the conversation as guest
// or the message sender is a bridged user
return this.$store.getters.getActorType() === 'guests' || this.actorType === ATTENDEE.ACTOR_TYPE.BRIDGED
Copy link
Member

Choose a reason for hiding this comment

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

Should be disabled unless the actor type is user?

Suggested change
return this.$store.getters.getActorType() === 'guests' || this.actorType === ATTENDEE.ACTOR_TYPE.BRIDGED
return this.$store.getters.getActorType() === 'guests' || this.actorType !== ATTENDEE.ACTOR_TYPE.USERS

Or do bots etc not fall into the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logic follows the same as we had on removed component
Code will be captured before it will reach NcAvatar for: guests, deleted users, bots, changelog, emails

@nickvergessen nickvergessen mentioned this pull request Nov 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@DorraJaouad DorraJaouad DorraJaouad approved these changes

Assignees

@Antreesy Antreesy

Labels
client: 💻 desktop feature: call 📹 Voice and video calls feature: conversations 👥 feature: frontend 🖌️ "Web UI" client regression technical debt
Projects
None yet
Milestone
v18.0.0-beta.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Antreesy @nickvergessen @DorraJaouad