Fix #17: per-sample description missing on first nav + scrollbar/schema fixes#20
Closed
TimPurdum wants to merge 1 commit into
Closed
Fix #17: per-sample description missing on first nav + scrollbar/schema fixes#20TimPurdum wants to merge 1 commit into
TimPurdum wants to merge 1 commit into
Conversation
The "About this sample" details block, per-page meta tags, and JSON-LD SoftwareSourceCode entry were missing on the first navigation from the navbar — they only appeared after a page refresh. The cause was a render-order race in MainLayout: the LocationChanged handler cleared LayoutService.CurrentPage, but on first navigation the layout could re-render with the new URL before the new SamplePage's OnInitialized had a chance to re-register itself. Replace the clear-on-navigation approach with a URI key on LayoutService — SamplePage records the URI it set CurrentPage for, and MainLayout's new EffectiveCurrentPage getter only treats CurrentPage as live when that URI matches NavigationManager.Uri. Stale references after navigating to a non-SamplePage route are silently ignored with no timing dependency. While there, enrich the per-page JSON-LD with keywords and mainEntity/mainEntityOfPage cross-references so each sample page emits visibly distinct schema.org content, and fix the unresolved double scrollbar on narrow screens by moving article.content's fixed height and overflow-y into the >=1075px media query (where <main> is set to overflow:hidden).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up fixes for #17 addressing Tim's testing feedback and the two unresolved Copilot review comments.
Tim's feedback
Root cause for both: the
MainLayout.OnLocationChangedhandler clearedLayoutService.CurrentPageon every navigation, expecting the nextSamplePage.OnInitializedto re-register. But on first nav from the navbar, the layout's render after the clear could land before the new SamplePage had run itsOnInitialized, so the description / SoftwareSourceCode / per-page meta tags rendered againstCurrentPage == null. That's also why the JSON-LD looked "static" — the per-page description / SoftwareSourceCode entry was missing on first paint.Fix: key
LayoutService.CurrentPageto the URI it was registered for.SamplePage.OnInitializednow passesNavigationManager.Uriwhen callingSetCurrentPage. MainLayout reads through a newEffectiveCurrentPagegetter that only returns the registered page when its URI matchesNavigationManager.Uri. No timing dependency, and stale references after navigating to a non-SamplePage route (Home/NotFound) are silently ignored — same guarantee the explicitClearCurrentPagewas trying to provide, without the race.While there, enrich the per-page JSON-LD with
keywordsderived from the page name plusmainEntity/mainEntityOfPagecross-references between WebPage and SoftwareSourceCode, so each sample page emits visibly distinct schema content beyond just url/name.Unresolved Copilot comments addressed
MainLayout.razor.css:63):article.contenthad a fixedheight: calc(100vh - 10rem)+overflow-y: autoat all widths, and on<1075pxso did the parent<main>— double scrollbars. Moved both into the>=1075pxmedia query, where<main>switches tooverflow:hiddenand inner article scrolling is what we want.CurrentPageafter navigating to a non-SamplePage route: subsumed by the URI-key approach above (the thread was marked resolved by the originalClearCurrentPagechange, but the new approach handles both this case and Tim's first-nav case with a single mechanism).Other Copilot comments were already addressed in the existing PR commits (
Dispose()no longer disposes the injectedHttpClient;summary::markercleared for cross-browser parity;MetaTitleonly appends "— GeoBlazor Sample" when on a sample page;SlugToTitletakes the last/-segment;CanonicalPageUrlstrips query/fragment).Files changed
LayoutService.cs— addPageUri;SetCurrentPage(page, uri); dropClearCurrentPage(no longer needed)SamplePage.cs— injectNavigationManager; passUriwhen registeringMainLayout.razor—EffectiveCurrentPagegetter; use it in render, meta tags, and JSON-LD;OnLocationChangednow just callsStateHasChanged; enrich JSON-LD per-pageMainLayout.razor.css— move article fixed-height + overflow into>=1075pxmedia queryTest plan
<meta name="description">,og:url, and JSON-LDWebPage/SoftwareSourceCodereflect the new sample on each navigation.<1075pxviewport, no nested/double scrollbars on sample pages with long content.>=1075px, article still scrolls independently inside main as before.Targets
feature/schema-org-jsonldso it stacks into #17.Generated by Claude Code