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

Additional Fix For #3868 - Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. #3884

Closed

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Feb 24, 2024

This is a fix on top of #3881 and resolves issues last discussed in issue #3868 by changing the conditional check to render mode rather than if there is a theme on page. This ensures the page is always refreshed in browser displaying the page theme that is set (or not then use site default) with only the correct css library while logged in administrator/host.

Changes:
if (!string.IsNullOrEmpty(childPage.ThemeType))
To:
if (PageState.RenderMode == RenderModes.Static)

image

We can come up with a more elaborate conditional, such as if navigating from pages that themes are not the same. However this solution always ensure the page will be loaded with fresh results if running in static render mode.

@thabaum thabaum changed the title Static render data enhance nav false Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. Feb 24, 2024
@thabaum thabaum changed the title Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. Fixes #3868 - Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. Feb 24, 2024
@thabaum thabaum changed the title Fixes #3868 - Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. Additional Fix For #3868 - Static Render hosting model sets navigation menu items theme control data-enhance-nav attribute to false. Feb 24, 2024
@sbwalker
Copy link
Member

sbwalker commented Feb 24, 2024

I cannot merge this PR as it would mean that every page navigation will do a full reload on static rendering which basically eliminates the performance benefit of enhanced navigation. The only time disabling enhanced navigation is necessary is if pages are using themes different from the site. There must be another explanation for the behavior for unauthenticated users.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 24, 2024

The issue is with an authenticated user. When unauthenticated it works as expected.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 24, 2024

Could it be one of the control panel controls?

@thabaum
Copy link
Contributor Author

thabaum commented Feb 24, 2024

@sbwalker The current behavior is very odd, when unauthenticated, all the pages in navigation links have this set to false. I have tried many different approaches with all types of results other than what I am expecting.

Pretty much the way I see it, the navigation links that are not sharing the same theme as the current page need to have this set to false.

If PageState.Page.ThemeType is the same as childPage.ThemeType it should not create this attribute. if it is different including the site defaults it should set this navigation attribute to false. Once on the new page navigation links need to be processed again on which pages get this attribute set to false.

It should be as simple as this:

if (!PageState.Page.ThemeType.Equals(childPage.ThemeType))

or

if (PageState.Page.ThemeType != childPage.ThemeType)

which wont make a link on the page you are on with this attribute, but still puts this link on other pages that have the same settings.

Way to test is create 2 pages, one with theme creator theme ,one with blazor theme and keep the other pages stock.

When visiting the pages with theme types set, the pages with same page theme do not need to be set with this attribute. All the other pages do need this set to force the refresh. And I think this is only an issue with static SSR.

I have tried writing that expression and a few others in a few different ways with same results.

The reason it currently works now is for some reason it sets this attribute on all pages, until you get logged in.

Not sure this makes sense as it is a bit tough to explain. The goal I am trying to achieve again is setting this attribute on all pages that dont use the same theme as the current page while navigating.

I tested a few things i thought should work but got odd results each time.

I also think that it should only worry about this if in static mode since it works fine in interactive mode so I believe my check in this PR is still valid concern or this will affect interactive mode as well.

I will review this more closer tomorrow as maybe something else is causing some odd behavior. If you look at source you can see which pages are getting what attributes set. Right now currently all of the pages I believe get this set until you log in. Could be a VS glitch on my end so I will start fresh on this tomorrow. Thought I would share what I thought might help.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 24, 2024

if (!PageState.Page.ThemeType.Equals(childPage.ThemeType))

This logic works, until you log in, then it just makes all links have this attribute.

I will update my PR with this new logic to test, but it acts strange and I feel there should be a conditional check to see if this is static render mode or not.
Logged in:
image
Logged out:
image

Notice homepage and "P1" page have no attributes added since I am on P1 page, My Page and My Portfolio both have page themes set blazor and theme creator themes.
And then if selecting a page with a theme:

image

Notice this logic works as expected when unauthenticated.

Once authenticated all pages in navigation are set to false except the page you are on. So something is wrong with the childPage.ThemeType or PageState.Page.ThemeType properties once logged in as the logic is not performing as it should.

if (PageState.RenderMode == RenderModes.Static && PageState.Page.ThemeType != childPage.ThemeType)
{ }

I feel this is the solution here, and another fix is needed for when logged in as another issue.

I will update this PR so you can test it

@sbwalker
Copy link
Member

@thabaum I appreciate all of your investigation into this issue. I included some additional context in #3868

@sbwalker sbwalker closed this Feb 24, 2024
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.

2 participants