-
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
Fix theme manager script #16386
Fix theme manager script #16386
Conversation
src/OrchardCore.Modules/OrchardCore.Themes/ResourceManagementOptionsConfiguration.cs
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.Themes/Assets/js/theme-toggler.js
Outdated
Show resolved
Hide resolved
@mdameer I updated some of my comments after addressed some of your comments and cleanup. So please check out my updated comments. |
src/OrchardCore.Modules/OrchardCore.Themes/ResourceManagementOptionsConfiguration.cs
Show resolved
Hide resolved
"Assets/js/theme-toggler.js" | ||
], | ||
"watch": [ | ||
"Assets/js/theme-toggler.js" | ||
], | ||
"output": "wwwroot/Scripts/theme-manager.js" |
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.
In this case where you separate the theme-head from the toggler, then I think theme-toggler.js
will be more clear, right?
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.
After cleanup the dependency, I thought theme-manager is better fit. Because I added dependency for the theme manager to the theme-head. So including the theme-manager will include the theme-head automatically. The reason I included them separately to ensure that the theme-head is loaded early on. So theme-manager would be fine.
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.
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.
Not sure why icons are not found. I don't experience this issue locally. I don't think this issue is related to this PR.
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.
Isn't the toolbox being unusable in dark mode one thing that #16356 is supposed to fix?
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.
yes and it does. but something on your local machine that is not discovering the icons.svg file which is the icons. I don't get the same behavior on my end. Every thing seems to be fine. I just tested it again.
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.
It's not discovering the SVG because it's not there under OrchardCore.Resources/Scripts/trumbowyg/ui/icons.svg. It's correctly a 404.
Isn't this file what you see as being requested?
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.
@Piedone the icons.svg file is available https://github.com/OrchardCMS/OrchardCore/tree/main/src/OrchardCore.Modules/OrchardCore.Resources/wwwroot/Scripts/ui
There is no issue on my side either. Check this out:
The dark theme:
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.
Note that what I see being requested is OrchardCore.Resources/Scripts/trumbowyg/ui/icons.svg
, under the trumbowyg
folder, and not OrchardCore.Resources/Scripts/ui/icons.svg
. But this gets messier by there being a second SVG under wwwroot/Styles/ui.
It loads the incorrect one because control arrives here:
The path to the Styles icon, what I'd expect to be actually used, is configured in two places, but this is not taking effect for me. This is because these two are only used if the editor for HtmlBody is configured as Trumbowyg instead of "Wysiwyg editor", what is the default. So, the toolbar will only work for you if you change the editor.
I think the fix is to move the SVG to the script, so the default detection will work (i.e. you're not forced to set $.trumbowyg.svgPath
), and remove the leftover other SVG. It doesn't seem to be referenced anywhere (like in CSS) so this should be fine. I committed this, please check and merge the PR if OK. It also fixes that the SVG in the wwwroot folder wasn't actually automatically copied, just I suppose updated by hand.
Also, for me, the toolbar is light even on main
when switching to the Trumbowyg editor:
But that's a mystery I'll solve later.
BTW something is broken with Resource Debug Mode, because even when it's force enabled, some, but not all, resources, including Trumbowyg, are loaded with their minified versions:
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.
I see! good catch. this issue was already in main and no one reported it. I also noticed that dark mode in the WYSIWYG template in HtmlBodyPart was not changing as I was trying to follow your steps above. I fix that as well.
I can also confirm that changing the resource debug mode, bootstrap.min.css
is still loaded instead of bootstrap.css
the rest seems to be loading as expected.
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.
Opened #16401 about this.
The PR #16356 moved some of the theme related scripts into
OrchardCore.Themes
but did not include all the theme reusable scripts.Here is a summary of the improvements
ResourceManifest
used in the Theme module is now static.theme-toggler
and themeconstants
into the Themes module to allow reusability.TheAdmin
andTheTheme
theme are able to use dark/light theme resources.theme-head
script which should be loaded early on to ensure to apply user's preferences early instead of waiting on the page to load.theme-manager
provides you a way to toggle theme selection./cc @mdameer