search

Microflash / vuepress-theme-succinct

A slightly opinionated theme for Vuepress
https://succinct.mflash.dev
MIT License
30 stars 3 forks source link

Add `ThemeManager` to `globalUIComponents` automatically #9

Closed kidonng closed 3 years ago

kidonng commented 3 years ago

So users don't have to add it manually.

naiyerasif commented 3 years ago

Not adding ThemeManager globally by default was a deliberate decision because I don't want to impose my opinions on people who use this theme and because it gives people control whether they want to use the ThemeSwitcher component or not. More importantly, since i18n is not enabled for this component (something that I've planned but sadly not getting enough bandwidth to work on), many people may want to avoid it for the sake of consistency.

I'd love to hear more why this could be a good idea to enable it by default.

kidonng commented 3 years ago

Not adding ThemeManager globally by default was a deliberate decision because I don't want to impose my opinions on people who use this theme and because it gives people control whether they want to use the ThemeSwitcher component or not.

When ThemeManager isn't added, the style is a little broken (mostly colors), so I think it doesn't make sense to not use ThemeManager with this theme.

More importantly, since i18n is not enabled for this component (something that I've planned but sadly not getting enough bandwidth to work on), many people may want to avoid it for the sake of consistency.

I noticed that but didn't realize it's the cause. I would like to help with i18n of the theme.

kidonng commented 3 years ago

Just found out the theme switching feature is using global variables (window.​__onThemeChange​ and so on), maybe it can refactored to use Vue instead? That way we may not need a separate <ThemeManger>.

naiyerasif commented 3 years ago

When ThemeManager isn't added, the style is a little broken (mostly colors), so I think it doesn't make sense to not use ThemeManager with this theme.

This is a bug where fallback variables are not being injected in :root when ThemeManager is not available. I've a fix for this that I'm planning to publish this week.

Just found out the theme switching feature is using global variables (window.​__onThemeChange​ and so on), maybe it can refactored to use Vue instead? That way we may not need a separate <ThemeManger>.

This can indeed be done and this will effectively eliminate the need for ThemeManager. However, if I do this, I'll have to handle theme-switching in the Layout.vue or Index.vue with which I'd also loose the ability to theme the parent-level (i.e., body level) scrollbar. Unthemed body level scrollbar looks pretty bad on dark theme which is the main reason why I'm sticking with window.​__onThemeChange​.