feat: add a banner area below the site header - #1564
Conversation
robinv8
left a comment
robinv8
left a comment
There was a problem hiding this comment.
Thanks for the PR — placing a banner below the site nav is a reasonable goal, and rendering it inside the React tree to survive hydration is the right approach. But after testing, I have to request changes:
-
This is a breaking change, not just a fix.
custom_headermoves from the top of<body>to below the nav. Existing sites rely on the current position (top announcement bars, scripts that must run before#root). Please call this out explicitly in the PR description — or better, keep the current position and add a separate slot below the nav so existing users are unaffected. -
Scripts in the custom header no longer execute. The old path goes through
ActivateScriptNodes, which re-creates<script>nodes so they run.dangerouslySetInnerHTMLnever executes scripts, so any header content containing JS silently breaks. Please activate script nodes after rendering the slot. -
Please revert the head/footer refactor. The
go-templateskip is not redundant: app settings are loaded before render (guard.ts:431), so on SSR pages the store is already populated at mount. The new unconditional injection clears the server-rendered markup andActivateScriptNodesexecutes every script a second time (analytics double-counting, etc.). Keeping the skip avoids this.
Happy to re-review once these are addressed.
The Header area writes its content at the top of <body>. That position is above the nav bar and outside #root. The nav bar and the page are both inside #root, and the injected node is a sibling of it. No stylesheet can move it below the nav, so a banner under the site header is not possible. Add a separate Banner area. The Header area keeps its position and its behaviour, so existing sites see no change. - Add custom_banner to SiteCustomCssHTMLReq, to the customize store and to the Admin CSS and HTML form. - Render CustomBanner below Header in pages/Layout. CustomSidebar renders custom_sidebar the same way. - Re-create the script nodes in the slot after render, so the browser runs them. Run them one time for each value. Wrap only a classic script, so a data block such as application/ld+json keeps its content, and a module keeps its own scope. The server template does not paint the banner. React uses createRoot, and the first commit of the root clears everything inside #root before Layout renders. A painted banner could not survive that, and its scripts would run one extra time.
37843c5 to
33e0caf
Compare
ngineer420
changed the title
|
@robinv8 Thanks for the detailed review. All three points are addressed, and the approach changed. The PR no longer moves
Your third point also showed that the first version was wrong in a second way. |
2 participants
Proposed Changes
Answer has a Header area in Admin → CSS and HTML. The client writes its content at the top of
<body>:The server template writes it in the same place, before
<div id="root">.That position is above the nav bar and outside
#root. The nav bar and the page are both inside#root, and the injected node is a sibling of#root. No stylesheet can move it below the nav. A banner under the site header is not possible today.This PR adds a new Banner area. It does not move the Header area.
custom_banneris a new field onSiteCustomCssHTMLReq, besidecustom_headerandcustom_sidebar.pages/Layoutrenders<CustomBanner />below<Header />.ui/template/header.html,internal/controller/template_controller.goandui/src/components/Customize/index.tsxare unchanged. The diff adds 136 lines and removes none.Answers to the review
1. Breaking change. Removed. The Header area keeps its position and its behaviour, so existing sites see no change. The banner is a separate optional field, and it stays empty until an admin fills it in.
CustomBannerrenders nothing while the field is empty.2. Scripts must run.
CustomBannerre-creates the script nodes after it renders the slot, so the browser runs them. It runs them one time for each value, and it tracks the value it activated. One difference fromActivateScriptNodes: it leaves a data block alone, such as a script with typeapplication/ld+json. The browser never runs a data block, and the IIFE wrapper would corrupt its content.3. Head and footer refactor. Reverted.
ui/src/components/Customize/index.tsxmatchesmainbyte for byte, and themeta[name="go-template"]skip stays.Why the server template does not paint the banner
The first version of this PR painted the banner in
header.htmland read that markup back in the client. That cannot work, and it would have caused the same double execution described in point 3 above.ui/src/index.tsxcallsReactDOM.createRoot, nothydrateRoot.Appreturns<InitialLoadingPlaceholder />on its first render, becauseuseMergeRoutesstarts with an empty route list and fills it in an effect. That placeholder is the first commit of the root, andcreateRootclears the container on the first commit. So React empties#rootbeforeLayoutrenders at all. Any banner the server painted inside#rootis already gone, and the browser has already run its scripts once while it parsed the document.The banner is therefore client-rendered only, in the same way as
custom_sidebarandCustomSidebar.#spin-maskcovers the viewport until React mounts, so a reader never sees the difference.Checks
go build ./...andgo vetpass.tsc --noEmit,eslintandprettier --checkpass on the changed files.swagv1.16.3. The ASF licence headers ondocs/docs.goanddocs/swagger.yamlare kept.