Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Jul 20, 2025

☑️ Resolves

Otherwise if no NcThemeProvider is used a runtime warning is shown in the dev tools console.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@susnux susnux requested review from Antreesy and ShGKme July 20, 2025 16:50
@susnux susnux added bug Something isn't working 3. to review Waiting for reviews labels Jul 20, 2025
@susnux susnux added this to the 9.0.0 milestone Jul 20, 2025
@susnux
Copy link
Contributor Author

susnux commented Jul 20, 2025

/backport to stable8

export function useIsDarkTheme(): DeepReadonly<Ref<boolean>> {
const isDarkTheme = useInternalIsDarkTheme()
const enforcedTheme = inject(INJECTION_KEY_THEME)
const enforcedTheme = inject(INJECTION_KEY_THEME, computed<''>(() => ''))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not undefined, to not create additional computed?

Suggested change
const enforcedTheme = inject(INJECTION_KEY_THEME, computed<''>(() => ''))
const enforcedTheme = inject(INJECTION_KEY_THEME, undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much better!

Copy link
Contributor

@Antreesy Antreesy Jul 21, 2025

Choose a reason for hiding this comment

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

Could you tell if it's worth making a new release?
From what I see locally, this one thing is killing performance (when many avatars rendered, 136 in this example):

before with , undefined added
image image

Copy link
Contributor

Choose a reason for hiding this comment

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

@Antreesy in production build?

Copy link
Contributor

Choose a reason for hiding this comment

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

So far noticed only while testing 9.0.0-rc.4 against this branch.
Maybe a result of #7157

@susnux susnux force-pushed the fix/theme-warning branch from 6c59210 to d2acae7 Compare July 21, 2025 13:32
Otherwise if no `NcThemeProvider` is used a runtime warning is shown in
the dev tools console.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/theme-warning branch from d2acae7 to 35829a4 Compare July 21, 2025 13:32
@susnux susnux merged commit 06e943f into main Jul 21, 2025
25 checks passed
@susnux susnux deleted the fix/theme-warning branch July 21, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants