-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Show ThemeToggler on supported themes only #17243
Conversation
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.
Was this causing any issues? Since nothing was displayed e.g. under TheBlog theme. Because if not, it would be easier to just permanently display it in Detail
, as it was (but not DetailAdmin
), rather than reimplementing the driver in every theme where we need it.
src/OrchardCore.Modules/OrchardCore.Themes/Drivers/ToggleThemeNavbarDisplayDriver.cs
Outdated
Show resolved
Hide resolved
It is showing up on every theme that uses the NavBar which is wrong because not every theme supports theme toggle. We should not by default expose it since the default implementation is for Bootstrap 5.3 only and not every theme uses bs 5.3 or want to support multiple themes. If someone designs a bs5.3 theme and wants to support theme toggle (like in TheTheme), they can add the toggler using a driver. But we should not just display it always. The issue that I ran into is that I am creating a theme that does not need toggler, but the toggler is showing up for me. |
OK, I see. |
@Piedone anything else to add here or should we merge it? |
See https://discord.com/channels/551136772243980291/1246954405014671513/1317165890181140560 I'll review soon. |
The Theme toggler (Dark/Auto/Light) should be explicitly added to themes that support it. By default it should not be added to every frontend theme.