Skip to content
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

OSOE-415: Orchard-Base-Theme shouldn't lock on every page load #52

Merged
merged 23 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9821b1a
Rewrite RemoveBootstrap5 to only lock when there is actually anything…
sarahelsaig Dec 8, 2022
41a7bcc
Make it a middleware instead.
sarahelsaig Dec 9, 2022
d161f2a
Don't async when done.
sarahelsaig Dec 9, 2022
0857456
Elaborate the xmldoc.
sarahelsaig Dec 9, 2022
6910f09
Update Lombiq.BaseTheme/Middlewares/RemoveBootstrapMiddleware.cs
sarahelsaig Dec 11, 2022
f9e2c45
Merge remote-tracking branch 'origin/dev' into issue/OSOE-415
sarahelsaig Dec 18, 2022
e85ffbe
Update site.css.map.
sarahelsaig Dec 18, 2022
500414b
Remove _done, because of false positive in multi-tenancy.
sarahelsaig Dec 18, 2022
5d4f6d0
Add notes about tenants.
sarahelsaig Dec 18, 2022
bd0a862
Add version to the bootstrap script so it doesn't break BehaviorChart…
sarahelsaig Dec 18, 2022
2c41be1
Apply suggestions from code review
sarahelsaig Dec 19, 2022
795417d
Remove only V5.
sarahelsaig Dec 19, 2022
f48a11e
Fix InvalidOperationException.
sarahelsaig Dec 19, 2022
3926f5b
Keep the latest version of script.
sarahelsaig Dec 20, 2022
c1f3f1f
simplify
sarahelsaig Dec 20, 2022
5217fd2
Upgrade bootstrap to 5.2.3.
sarahelsaig Dec 20, 2022
b67b2ac
Update wwwroot.
sarahelsaig Dec 20, 2022
2095f43
Fix BS 5.2 navbar breaking change.
sarahelsaig Dec 20, 2022
e07e84b
Update bootstrap version in samples project too.
sarahelsaig Dec 20, 2022
39964c7
Fix bug with RemoveBootstrapMiddleware.
sarahelsaig Dec 20, 2022
22235a8
Use the JS bundle (contains popper).
sarahelsaig Dec 20, 2022
effdaaa
Add comment in GetResourcesToClear.
sarahelsaig Dec 20, 2022
c76f164
Tighten relationship between references to NPM BS version.
sarahelsaig Dec 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Lombiq.BaseTheme.Samples/wwwroot/css/site.css.map

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion Lombiq.BaseTheme/Manifest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@
Author = "Lombiq Technologies",
Version = "0.0.1",
Website = "https://github.com/Lombiq/Orchard-Base-Theme",
Description = "The base frontend theme for shared content that are not specific to a specific project's theme."
Description = "The base frontend theme for shared content that are not specific to a specific project's theme." +
sarahelsaig marked this conversation as resolved.
Show resolved Hide resolved
"Warning: themes using this as the base remove the stock Bootstrap resource. If you switch to a different " +
"theme, please reload the tenant from Configuration > Tenants in the admin menu."
sarahelsaig marked this conversation as resolved.
Show resolved Hide resolved
)]
19 changes: 4 additions & 15 deletions Lombiq.BaseTheme/Middlewares/RemoveBootstrapMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace Lombiq.BaseTheme.Middlewares;
/// Themes derived from Lombiq.BaseTheme use Bootstrap from NPM as it's compiled into their site stylesheet. So this
/// duplicate resource is not needed and can cause problems if not removed. This situation can arise when a module
/// (such as Lombiq.DataTables) depends on Bootstrap and doesn't explicitly depend on Lombiq.BaseTheme so the built-in
/// resource would injected if this middleware didn't remove it.
/// resource would be injected if this middleware didn't remove it.
/// </summary>
public class RemoveBootstrapMiddleware
{
Expand All @@ -25,34 +25,25 @@ public class RemoveBootstrapMiddleware

private readonly RequestDelegate _next;

private static bool _done;

public RemoveBootstrapMiddleware(RequestDelegate next) => _next = next;

public Task InvokeAsync(
public async Task InvokeAsync(
HttpContext context,
IOptions<ResourceManagementOptions> resourceManagementOptions,
ISiteThemeService siteThemeService) =>
_done
? _next(context)
: InvokeInnerAsync(context, resourceManagementOptions.Value, siteThemeService);

private async Task InvokeInnerAsync(
HttpContext context,
ResourceManagementOptions resourceManagementOptions,
ISiteThemeService siteThemeService)
{
if (IsCurrentTheme(await siteThemeService.GetSiteThemeAsync()))
{
var resourcesToClear = resourceManagementOptions
.Value
.ResourceManifests
.Where(manifest => !manifest.GetResources("$" + nameof(FeatureIds.Area)).ContainsKey(FeatureIds.Area))
.SelectMany(manifest => _resourceTypes
.SelectWhere(manifest.GetResources)
.SelectWhere(resources => resources.GetMaybe("bootstrap"), resource => resource?.Any() == true))
.ToList();

// This only happens once per process instance, so locking would be rare.
// This only happens once per tenant process instance, so locking is rare.
if (resourcesToClear.Any()) ClearResources(resourcesToClear);
}

Expand All @@ -71,8 +62,6 @@ private static void ClearResources(IEnumerable<IList<ResourceDefinition>> resour
{
resource.Clear();
}

_done = true;
}
}
}
3 changes: 2 additions & 1 deletion Lombiq.BaseTheme/ResourceManagementOptionsConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ static ResourceManagementOptionsConfiguration()

_manifest
.DefineScript("bootstrap")
.SetUrl(Vendors + "bootstrap/js/bootstrap.min.js", Vendors + "bootstrap/js/bootstrap.js");
.SetUrl(Vendors + "bootstrap/js/bootstrap.min.js", Vendors + "bootstrap/js/bootstrap.js")
.SetVersion("5.1.3");
}

public void Configure(ResourceManagementOptions options) => options.ResourceManifests.Add(_manifest);
Expand Down
2 changes: 1 addition & 1 deletion Lombiq.BaseTheme/wwwroot/css/site.css.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Use this as the base theme of any custom frontend themes you create. For instruc

The theme makes use of the [ICssClassHolder](Services/ICssClassHolder.cs) service which provides a scoped container for adding class names from your own code. Use the provided zone names in the [ZoneNames](Constants/ZoneNames.cs) static class to address it.

You may have noticed, that we mentioned Bootstrap 5.1, even though your version of Orchard Core may be still using Bootstrap 5.0. This theme automatically removes the built-in Bootstrap resource manifests and replaces them with the vendor's JavaScript file pulled from NPM. As the Bootstrap stylesheet is already bundled into the site stylesheet there is no need to include that in the resource manifest.
You may have noticed, that we mentioned Bootstrap 5.1, even though your version of Orchard Core may be still using Bootstrap 5.0. This theme automatically removes the built-in Bootstrap resource manifests on the current tenant and replaces them with the vendor's JavaScript file pulled from NPM. As the Bootstrap stylesheet is already bundled into the site stylesheet there is no need to include that in the resource manifest. If you want to switch over to a different theme that doesn't use this as its base, please reload your tenant by going to Admin > Configuration > Tenants and clicking on the current tenant's Reload button.
sarahelsaig marked this conversation as resolved.
Show resolved Hide resolved

Besides the style and layout, the theme also automatically includes a minimalist helper script that eases transition away from jQuery. You don't really need full jQuery now that Internet Explorer is effectively dead (Internet Explorer 11 is going end of life on June 15, 2022 so you should not support it in any new project at this time). The script gives you the `window.helper` object. You can use `helper.ready(($) => {})` in your scripts, where `$(querySelector, baseElement)` returns a JavaScript `Array` of `Element`s.

Expand Down