Add cascading parameters for theme views#74
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a breaking architectural change that switches theme view base types from direct [Parameter] bindings to [CascadingParameter] bindings, enabling theme components to receive context values (Documents, Document, Plugins, Theme, Site) via a cascading layout wrapper instead of explicit component attributes.
Key Changes:
- Introduced a builder pattern for application initialization (
ScissorHandsApplicationBuilder) replacing the genericScissorHandsApplication<T1, T2, T3, T4, T5>with a non-generic version that uses reflection - Converted all theme view base class properties from
[Parameter]to[CascadingParameter]and made them nullable - Added
CascadingMainLayoutBasewrapper component to provide cascading values throughout the component tree - Refactored document rendering to use parallel processing with
Task.WhenAll
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ScissorHands.Web/ScissorHandsApplicationBuilder.cs | New builder class providing fluent API for configuring layouts with validation |
| src/ScissorHands.Web/ScissorHandsApplication.cs | Refactored to non-generic implementation using reflection to invoke BuildAsync with runtime types |
| src/ScissorHands.Web/Renderers/ComponentRenderer.cs | Updated to filter out cascading parameters from component attributes to prevent duplicate bindings |
| src/ScissorHands.Web/Generators/StaticSiteGenerator.cs | Refactored document rendering to use parallel processing and extracted rendering logic into separate methods |
| src/ScissorHands.Theme/IndexViewBase.cs | Changed Documents, Plugins, Theme, Site from Parameter to CascadingParameter and made nullable |
| src/ScissorHands.Theme/PageViewBase.cs | Changed Document, Plugins, Theme, Site from Parameter to CascadingParameter and made nullable |
| src/ScissorHands.Theme/PostViewBase.cs | Changed Document, Plugins, Theme, Site from Parameter to CascadingParameter and made nullable |
| src/ScissorHands.Theme/NotFoundViewBase.cs | Changed Document, Plugins, Theme, Site from Parameter to CascadingParameter and made nullable |
| src/ScissorHands.Web/themes/default/MainLayout.razor | Wrapped body with CascadingMainLayoutBase to provide cascading values |
| src/ScissorHands.Web/themes/default/IndexView.razor | Updated to use null-conditional operators for Documents property |
| src/ScissorHands.Web/themes/default/PageView.razor | Updated to use null-conditional and null-forgiving operators for Document.Html |
| src/ScissorHands.Web/themes/default/PostView.razor | Updated to use null-conditional and null-forgiving operators for Document.Html |
| src/ScissorHands.Web/themes/default/NotFoundView.razor | Updated null-checking logic for Document.Html property |
| src/ScissorHands.Web/themes/default/_Imports.razor | Added using directive for ScissorHands.Theme namespace |
| test/ScissorHands.Theme.Tests/IndexViewBaseTests.cs | Updated tests to expect null Documents and added test for cascading parameter binding |
| test/ScissorHands.Theme.Tests/PageViewBaseTests.cs | Updated tests to expect null Document and added test for cascading parameter binding |
| test/ScissorHands.Theme.Tests/PostViewBaseTests.cs | Updated tests to expect null Document and added test for cascading parameter binding |
| test/ScissorHands.Theme.Tests/NotFoundViewBaseTests.cs | Updated tests to expect null Document and added test for cascading parameter binding |
| test/ScissorHands.Web.Tests/Renderers/ComponentRendererCascadingParametersTests.cs | New test file verifying cascading parameters flow through layout without being passed as component attributes |
| README.md | Updated usage example to demonstrate new builder pattern API |
Comments suppressed due to low confidence (1)
src/ScissorHands.Web/themes/default/NotFoundView.razor:15
- The null-checking logic for Document is incorrect. At line 5,
string.IsNullOrWhiteSpace(Document?.Html) == truewill be true if Document is null OR if Document.Html is null/whitespace. However, in the else branch at line 15, the code accessesDocument.Htmlwithout a null-conditional operator or null-forgiving operator.
If Document is null, line 15 will throw a NullReferenceException. The code should use Document?.Html! or add an additional null check for Document itself.
@if (string.IsNullOrWhiteSpace(Document?.Html) == true)
{
<article>
<h1>404 - Not Found</h1>
<p>The page you are looking for does not exist.</p>
</article>
}
else
{
<article>
@((MarkupString)Document.Html)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@justinyoo I've opened a new pull request, #75, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@justinyoo I've opened a new pull request, #76, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@justinyoo I've opened a new pull request, #77, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Fix null-checking logic in IndexView.razor Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com>
…75) * Initial plan * Refactor ComponentRenderer to use reflection for cascading parameter detection Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> * Improve reflection to auto-discover cascading parameters from entire Theme assembly Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> * Add error handling and use GetExportedTypes for safer reflection Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com>
…dling (#77) * Initial plan * Add robust error handling and caching for reflection-based method invocation Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> * Add thread-safety and improve exception handling Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> * Address final code review feedback - constants and stack trace preservation Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: justinyoo <1538528+justinyoo@users.noreply.github.com>
Purpose
IndexViewBase,PageViewBase,PostViewBase,NotFoundViewBase) from direct[Parameter]inputs to[CascadingParameter]so themes can receive context consistently via the layout.CascadingMainLayoutBase.Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
README updated?
The top-level readme for this repo contains a link to each sample in the repo. If you're adding a new sample did you update the readme?
How to Test
What to Check
Verify that the following are valid
Document(s),Plugins,Theme, andSitevia cascading parameters when rendered under the main layout.Other Information
CascadingMainLayoutBase(or otherwise provide equivalent cascading values) to populate the view base properties.