perf(utils): ensure only 64px or 512px avatars are loaded #6749
Conversation
susnux
added
bug
|
/backport to stable8 |
| const root = useTemplateRef('main') | ||
| const isDarkTheme = useIsDarkThemeElement(root) |
There was a problem hiding this comment.
Is there a need to check specifically main and not body and thus use shared composable?
If so, I would at least go with props.menuContainer, so it inherits it from container
There was a problem hiding this comment.
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.
Is there a need to check specifically
mainand notbodyand thus use shared composable?
To avoid misunderstanding, main here is template ref key, not <main> query selector
There was a problem hiding this comment.
how does talk handle this currently?
<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 lib
There was a problem hiding this comment.
We remount the conversation avatar in the top bar
The backend only supports those values, so loading any other size will result in one of them but will be cached under a different URL by the browser. This should reduce the avatar queries of the browser to only max. 2 per user. Signed-off-by: Ferdinand Thiessen <[email protected]>
|
|
||
| setup() { | ||
| const root = useTemplateRef('root') | ||
| const isDarkTheme = useIsDarkThemeElement(root) |
There was a problem hiding this comment.
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 useIsDarkTheme by default and useIsDarkThemeElement only if specified explicitly?
There was a problem hiding this comment.
Maybe instead of [data-theme-dark] for override theme we provide <NcThemeProvider /> or useDarkTheme that provides the overridden dark theme?
In this case we don't need a mutation observer and can rely on JS only.
There was a problem hiding this comment.
provide inject in the composable?
There was a problem hiding this comment.
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.
| // Watch for element changes | ||
| watch(element, updateIsDarkTheme) |
There was a problem hiding this comment.
Is there a use case for this watch?
| 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}`, { |
There was a problem hiding this comment.
Should it be in nextcloud/router?
Co-authored-by: Grigorii K. Shartsev <[email protected]> Signed-off-by: Ferdinand Thiessen <[email protected]>
☑️ Resolves
The backend only supports those values,
so loading any other size will result in one of them but will be cached under a different URL by the browser. This should reduce the avatar queries of the browser to only max. 2 per user.
🏁 Checklist
stable8for maintained Vue 2 version.