-
Notifications
You must be signed in to change notification settings - Fork 94
perf(utils): ensure only 64px or 512px avatars are loaded #6749
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
--> | ||
|
||
<template> | ||
<div class="autocomplete-result"> | ||
<div ref="root" class="autocomplete-result"> | ||
<!-- Avatar or icon --> | ||
<div :class="[icon, `autocomplete-result__icon--${avatarUrl ? 'with-avatar' : ''}`]" | ||
:style="avatarUrl ? { backgroundImage: `url(${avatarUrl})` } : null " | ||
|
@@ -31,6 +31,8 @@ | |
</template> | ||
|
||
<script> | ||
import { useTemplateRef } from 'vue' | ||
import { useIsDarkThemeElement } from '../../composables/useIsDarkTheme/index.ts' | ||
import { getAvatarUrl } from '../../utils/getAvatarUrl.ts' | ||
|
||
import NcUserStatusIcon from '../NcUserStatusIcon/index.js' | ||
|
@@ -81,25 +83,30 @@ export default { | |
default: () => ({}), | ||
}, | ||
}, | ||
|
||
setup() { | ||
const root = useTemplateRef('root') | ||
const isDarkTheme = useIsDarkThemeElement(root) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 99% of cases we need only the entire document dark theme. And in rare cases we need to check the avatar itself (for example, in the top bar in Talk during a call, because the call view is always aka dark). What about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for other components There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe instead of In this case we don't need a mutation observer and can rely on JS only. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. provide inject in the composable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or in the component provider, both are fine for me. <!-- before -->
<div data-theme-dark>
...content...
</div>
<!-- after -->
<NcThemeProvider dark>
...content...
</NcThemeProvider> Only for the cases when we override the theme. I worry about it because overriding the theme is quite a rare case, but to support it this PR adds observer for every avatar instance, and we may have a lot. |
||
return { | ||
isDarkTheme, | ||
} | ||
}, | ||
|
||
computed: { | ||
avatarUrl() { | ||
if (this.iconUrl) { | ||
return this.iconUrl | ||
} | ||
|
||
return this.id && this.source === 'users' | ||
? this.getAvatarUrl(this.id, 44) | ||
? getAvatarUrl(this.id, { isDarkTheme: this.isDarkTheme }) | ||
: null | ||
}, | ||
// For backwards compatibility | ||
labelWithFallback() { | ||
return this.label || this.title | ||
}, | ||
}, | ||
|
||
methods: { | ||
getAvatarUrl, | ||
}, | ||
} | ||
</script> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,8 @@ | |
* SPDX-License-Identifier: AGPL-3.0-or-later | ||
*/ | ||
|
||
import type { DeepReadonly, Ref } from 'vue' | ||
import { ref, readonly, watch } from 'vue' | ||
import type { DeepReadonly, MaybeRef, Ref } from 'vue' | ||
import { ref, readonly, watch, toValue, computed } from 'vue' | ||
import { createSharedComposable, usePreferredDark, useMutationObserver } from '@vueuse/core' | ||
import { checkIfDarkTheme } from '../../functions/isDarkTheme/index.ts' | ||
|
||
|
@@ -15,19 +15,22 @@ | |
* @param el - The element to check for the dark theme enabled on (default is `document.body`) | ||
* @return {DeepReadonly<Ref<boolean>>} - computed boolean whether the dark theme is enabled | ||
*/ | ||
export function useIsDarkThemeElement(el: HTMLElement = document.body): DeepReadonly<Ref<boolean>> { | ||
const isDarkTheme = ref(checkIfDarkTheme(el)) | ||
export function useIsDarkThemeElement(el: MaybeRef<HTMLElement> = document.body): DeepReadonly<Ref<boolean>> { | ||
const element = toRef(el) | ||
Check failure on line 19 in src/composables/useIsDarkTheme/index.ts
|
||
const isDarkTheme = ref(checkIfDarkTheme(toValue(el))) | ||
const isDarkSystemTheme = usePreferredDark() | ||
|
||
/** Update the isDarkTheme */ | ||
function updateIsDarkTheme() { | ||
isDarkTheme.value = checkIfDarkTheme(el) | ||
isDarkTheme.value = checkIfDarkTheme(element.value) | ||
} | ||
|
||
// Watch for element change to handle data-theme* attributes change | ||
useMutationObserver(el, updateIsDarkTheme, { attributes: true }) | ||
useMutationObserver(element, updateIsDarkTheme, { attributes: true }) | ||
// Watch for system theme change for the default theme | ||
watch(isDarkSystemTheme, updateIsDarkTheme, { immediate: true }) | ||
// Watch for element changes | ||
watch(element, updateIsDarkTheme) | ||
Comment on lines
+32
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a use case for this watch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Element updated? |
||
|
||
return readonly(isDarkTheme) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,47 @@ | |
*/ | ||
|
||
import { generateUrl } from '@nextcloud/router' | ||
import { checkIfDarkTheme } from '../functions/isDarkTheme/index.ts' | ||
|
||
export const getAvatarUrl = (user: string, size: number | string, isGuest?: boolean): string => { | ||
const darkTheme = window.getComputedStyle(document.body) | ||
.getPropertyValue('--background-invert-if-dark') === 'invert(100%)' | ||
interface AvatarUrlOptions { | ||
/** | ||
* Should the dark theme variant be used. | ||
*/ | ||
isDarkTheme?: boolean | ||
|
||
return generateUrl('/avatar' + (isGuest ? '/guest' : '') + '/{user}/{size}' + (darkTheme ? '/dark' : ''), { | ||
/** | ||
* Is the user a guest user. | ||
*/ | ||
isGuest?: boolean | ||
|
||
/** | ||
* Size of the avatar. | ||
* @default 64 | ||
*/ | ||
size?: 64 | 512 | ||
} | ||
|
||
/** | ||
* Get the avatar URL for a given user. | ||
* | ||
* @param user - The user id | ||
* @param options - Adjustments for the avatar format | ||
*/ | ||
export function getAvatarUrl(user: string, options?: AvatarUrlOptions): string { | ||
// backend only supports 64 and 512px | ||
// so we only requrest the needed size for better caching of the request. | ||
const size = (options?.size || 64) <= 64 | ||
? 64 | ||
: 512 | ||
|
||
const guestUrl = options?.isGuest | ||
? '/guest' | ||
: '' | ||
const themeUrl = options?.isDarkTheme ?? checkIfDarkTheme(document.body) | ||
? '/dark' | ||
: '' | ||
|
||
return generateUrl(`/avatar${guestUrl}/{user}/{size}${themeUrl}`, { | ||
Comment on lines
+33
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be in |
||
user, | ||
size, | ||
}) | ||
|
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.
Is there a need to check specifically
main
and notbody
and thus use shared composable?If so, I would at least go with
props.menuContainer
, so it inherits it from containerThere 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 otherwise it does not work if you use the avatar within a forced theme.
This can not be influenced by the app developer so we need to do this here.
(how does talk handle this currently? IIRC you use force dark theme during calls?)
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.
To avoid misunderstanding,
main
here is template ref key, not<main>
query selectorUh oh!
There was an error while loading. Please reload this page.
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.
<div class="top-bar__wrapper" :data-theme-dark="isInCall">
🙈But we do not rewrite avatars in call, if I'm not mistaken, only rely on
useIsDarkTheme()
composable from the libThere 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.
We remount the conversation avatar in the top bar