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

[ENH] - Add Prerender IModuleControl property (similar to RenderMode)… #4179

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

vnetonline
Copy link
Contributor

@vnetonline vnetonline commented Apr 23, 2024

#4178

@sbwalker I had a go at adding Prerender just like RenderMode, I hope my implementation meets your standards let me know if you would like me to modify anything

@sbwalker
Copy link
Member

sbwalker commented Apr 23, 2024

@vnetonline this looks good... the only question I have is related to the default behavior:

Currently there is a Site setting for Prerender which controls whether or not pre-rendering is enabled for the entire site. Currently it is possible to set Prerendering to False at the Site level and only enable Prerendering for some specific components (ie. only some Interactive components on public facing pages which need to be indexed for SEO purposes). Based on this PR, this scenario would no loinger be possible due to the default value and logic below:

ModuleState.Prerender != false ? PageState.Site.Prerender : ModuleState.Prerender

Basically this says that if the component-level property is set to True then it should use the Site Setting - Prerender property setting (which would be False in the case I described above). Perhaps a more appropriate solution would be if the component-level Prerender property was a nullable boolean type and had a default value of null. Then the logic would be able to determine if the value was overridden or not:

ModuleState.Prerender == null ? PageState.Site.Prerender : ModuleState.Prerender

The above logic uses the Site Setting unless it is explicitly overridden at the component level.

Thoughts?

@vnetonline
Copy link
Contributor Author

vnetonline commented Apr 23, 2024

@sbwalker, the above logic sounds good, I would reverse it from a conventional perspective, it's more readable like this

ModuleState.Prerender != null ? ModuleState.Prerender : PageState.Site.Prerender

I will modify the PR and submit it again

@vnetonline
Copy link
Contributor Author

@sbwalker I have tried to do as you suggest, but I haven't been successful because

GetInteractiveRenderMode method doesn't accept nullable boolean type

when you say that "only enable Prerendering for some specific components" where does this happen ??

@vnetonline
Copy link
Contributor Author

vnetonline commented Apr 23, 2024

@sbwalker i can do what you suggested using below code block ... but it doesn't look elegant enough

if (ModuleState.Prerender == false)
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, false)" />

} 
else if (ModuleState.Prerender == true)
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, true)" />
}
else
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, 
        PageState.Site.Prerender)"/>
}

@sbwalker sbwalker merged commit 4cf2b74 into oqtane:dev Apr 24, 2024
1 check passed
sbwalker added a commit to sbwalker/oqtane.framework that referenced this pull request Apr 24, 2024
sbwalker added a commit that referenced this pull request Apr 24, 2024
@vnetonline
Copy link
Contributor Author

@sbwalker i can do what you suggested using below code block ... but it doesn't look elegant enough

if (ModuleState.Prerender == false)
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, false)" />

} 
else if (ModuleState.Prerender == true)
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, true)" />
}
else
{
    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="@InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, 
        PageState.Site.Prerender)"/>
}

Simplefied Version

    var rendermode = InteractiveRenderMode.GetInteractiveRenderMode(PageState.Site.Runtime, ModuleState.Prerender ? 
       PageState.Site.Prerender);

    <RenderModeBoundary ModuleState="@ModuleState" PageState="@PageState" SiteState="@SiteState" 
        @rendermode="rendermode" />

@sbwalker
Copy link
Member

@vnetonline the PR is already merged

@vnetonline
Copy link
Contributor Author

@vnetonline the PR is already merged

Yes thank you was just adding a comment as to what changed in this commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants