Skip to content

fix(#3762): AppHeader Slots#3909

Open
willcodeforcoffee wants to merge 1 commit into
devfrom
eric/bug3762-AppHeader-slots
Open

fix(#3762): AppHeader Slots#3909
willcodeforcoffee wants to merge 1 commit into
devfrom
eric/bug3762-AppHeader-slots

Conversation

@willcodeforcoffee

@willcodeforcoffee willcodeforcoffee commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Before (the change)

Our AppHeader examples are using a slot property on some of our components, design.alberta.ca/components/app-header#tab-1, specifically, the Header with Navigation example.
See:

  1. https://design.alberta.ca/components/app-header/
  2. https://design.alberta.ca/examples/header-with-navigation/

After (the change)

Several changes have been made to make these controls more consistent with other controls.

  • React: Added props for banner, phase, navigation, and utilities slots.
  • Angular: Added template attributes for banner, phase, navigation, and utilities slots.
  • Renamed Angular files from header to app-header and header-menu to app-header-menu
  • All React and Angular examples updated to use appropriate prop/templates
  • Other documentation updated as well

Backward compatibility with slots is maintained.

Make sure that you've checked the boxes below before you submit the PR

  • I have read and followed the setup steps
  • I have created necessary unit tests
  • I have tested the functionality in both React and Angular.

Steps needed to test

  • Note: There are some problems with the PRs page:
    • The PR page is not resizable so you can't test mobile versions
    • There seems to be an issue a separate issue where window.innerWidth increases for the different Headers on the PR page
    • Try using the docs page where you can http://localhost:4201/docs/app-header for testing different screen widths
  • backward compatibility maintained
  • props for React components work as expected
  • templates for Angular components work as expected
  • documentation is correct and makes sense

@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 681a117 to c525784 Compare May 8, 2026 15:55
@willcodeforcoffee willcodeforcoffee changed the base branch from main to dev May 8, 2026 16:13
@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://GovAlta.github.io/ui-components/pr-preview-angular/pr-3909/

Built to branch gh-pages at 2026-06-05 15:22 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown

Preview links

Target URL
Docs https://govalta.github.io/ui-components/pr-preview/pr-3909/
React playground https://govalta.github.io/ui-components/pr-preview-react/pr-3909/
Angular playground https://govalta.github.io/ui-components/pr-preview-angular/pr-3909/

Built from commit e6c4f3e. Previews are removed automatically when this PR closes.

@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 524280a to 8685814 Compare May 11, 2026 20:07
@willcodeforcoffee willcodeforcoffee self-assigned this May 11, 2026
@willcodeforcoffee willcodeforcoffee added Bug Something isn't working P2 Priority 2 (should have): Improves completeness and reduces the post-launch support load. labels May 11, 2026
@willcodeforcoffee willcodeforcoffee marked this pull request as ready for review May 11, 2026 20:32
@willcodeforcoffee willcodeforcoffee requested a review from Copilot May 12, 2026 15:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the AppHeader slot APIs and examples to make React/Angular usage more consistent with other controls (adding explicit “banner/phase/navigation/utilities” APIs), and updates docs/examples accordingly while aiming to preserve legacy slot-based usage.

Changes:

  • React: adds banner, phase, navigation, and utilities props to GoabAppHeader.
  • Angular: adds banner, phase, navigation, and utilities template inputs to GoabAppHeader, and introduces GoabAppHeaderMenu.
  • Docs & PR apps: updates configuration snippets and example pages to use the new APIs, plus adds bug reproduction routes for #3762.

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
libs/react-components/src/lib/app-header/app-header.tsx Adds new slot props and renders them into goa-app-header.
libs/angular-components/src/lib/components/index.ts Re-exports app-header / app-header-menu under new folder names.
libs/angular-components/src/lib/components/app-header/app-header.ts Adds TemplateRef inputs and projects them into named slots.
libs/angular-components/src/lib/components/app-header/app-header.spec.ts Updates spec import path after renaming.
libs/angular-components/src/lib/components/app-header-menu/app-header-menu.ts Adds Angular wrapper for goa-app-header-menu including slotName host binding.
libs/angular-components/src/lib/components/app-header-menu/app-header-menu.spec.ts Updates spec import path and minor formatting.
docs/src/data/configurations/app-header.ts Updates AppHeader code snippets to use new React props / Angular template inputs.
docs/src/data/configurations/app-header-menu.ts Updates AppHeaderMenu snippets (React/Angular/Web Components).
docs/src/content/examples/header-with-navigation/react.tsx Updates docs example to use navigation/utilities props.
docs/src/content/examples/header-with-navigation/angular.html Updates docs example to use Angular template inputs.
docs/generated/component-apis/app-header.json Updates generated API docs for AppHeader slots.
apps/prs/react/src/routes/docs/app-header/app-header.tsx Updates PR app docs route examples to new React props.
apps/prs/react/src/routes/bugs/bug3762.tsx Adds React PR bug route demonstrating new vs legacy slot usage.
apps/prs/react/src/app/routes/bugs/bug3762.route.ts Registers React PR bug route for #3762.
apps/prs/angular/src/routes/docs/app-header/app-header.component.html Updates PR app Angular docs route examples to template inputs.
apps/prs/angular/src/routes/bugs/3762/bug3762.route.json Registers Angular PR bug route metadata for #3762.
apps/prs/angular/src/routes/bugs/3762/bug3762.component.ts Adds Angular PR bug route component shell for #3762.
apps/prs/angular/src/routes/bugs/3762/bug3762.component.html Adds Angular PR bug route template demonstrating new vs legacy slot usage.
Comments suppressed due to low confidence (3)

libs/angular-components/src/lib/components/app-header/app-header.ts:45

  • navigation is projected as a single wrapper <div slot="navigation">…</div>. goa-app-header v2 navigation expects multiple direct slotted items (it counts host.children[slot="navigation"] and styles via ::slotted(a) / ::slotted(goa-app-header-menu)), so wrapping will break styling and overflow behavior. Prefer slotting each nav item directly (or update the web component to support nested wrappers).
    libs/angular-components/src/lib/components/app-header/app-header.ts:49
  • utilities is projected as a single wrapper <div slot="utilities">…</div>. In goa-app-header v2, utilities responsiveness depends on counting/measuring direct slotted utility elements (host.children with slot="utilities"); wrapping multiple actions into one element will be treated as a single item and can disable the intended “utilities menu” behavior and ::slotted(a) styling.
    libs/angular-components/src/lib/components/app-header/app-header.ts:76
  • banner/phase/navigation/utilities inputs introduce new rendering paths (NgTemplateOutlet + slot projection), but the existing spec doesn’t assert these slots are rendered correctly. Add Angular unit tests that bind each template input and verify the expected slotted output under goa-app-header.

Comment thread libs/react-components/src/lib/app-header/app-header.tsx
Comment thread libs/react-components/src/lib/app-header/app-header.tsx Outdated
Comment thread docs/src/data/configurations/app-header-menu.ts
Comment thread docs/src/data/configurations/app-header-menu.ts Outdated
Comment thread apps/prs/react/src/routes/bugs/bug3762.tsx Outdated
Comment thread apps/prs/angular/src/routes/bugs/3762/bug3762.component.html Outdated
Comment thread docs/src/data/configurations/app-header.ts
Comment thread apps/prs/angular/src/routes/docs/app-header/app-header.component.html Outdated
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 8685814 to dd42741 Compare May 12, 2026 19:27
@willcodeforcoffee

Copy link
Copy Markdown
Collaborator Author

Copilot Feedback Addressed:

  • Added unit tests for the slots in Angular and React components
  • removed unreachable code in docs
  • formatting and other stuff

Comment thread libs/angular-components/src/lib/components/app-header/app-header.ts
Comment thread libs/angular-components/src/lib/components/app-header/app-header.spec.ts Outdated
Comment thread libs/react-components/src/lib/app-header/app-header.tsx
Comment thread apps/prs/angular/src/routes/bugs/3762/bug3762.component.html Outdated
Comment thread apps/prs/react/src/routes/bugs/bug3762.tsx
Comment thread apps/prs/react/src/routes/docs/app-header/app-header.tsx
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 66e675b to 3efa868 Compare May 20, 2026 21:11
@willcodeforcoffee

Copy link
Copy Markdown
Collaborator Author

Updated the PR with the following:

  • Removed "V2 only" from all prop comments, they're not necessary anymore
  • Updated tests to use beforeEach
  • Testing for content within slots instead of not.null
  • Programatically moved slotted content up in the DOM so that the Navigation Menu can calculate menu overflow, also fixed how the menu appears, but also works as expected in other React and Angular components
  • Fixed utilities slot examples in docs since containing div is gone

Comment thread apps/prs/angular/src/routes/bugs/3762/bug3762.component.html Outdated
Comment thread apps/prs/react/src/routes/docs/app-header/app-header.tsx
Comment thread apps/prs/react/src/routes/docs/app-header/app-header.tsx
Comment thread apps/prs/react/src/routes/docs/app-header/app-header.tsx
@ArakTaiRoth ArakTaiRoth linked an issue May 26, 2026 that may be closed by this pull request
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from eb55151 to 4d77773 Compare May 28, 2026 15:26
@willcodeforcoffee

Copy link
Copy Markdown
Collaborator Author

I've made the following changes:

  • made a fix to adding items in the navigation slot
  • made a fix to items in the utilities slot
  • made a fix to how the menu shows on the Angular component
  • squashed onto dev so hopefully specs pass in CI

@bdfranck bdfranck requested a review from vanessatran-ddi May 29, 2026 15:59
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 4d77773 to 7b67b29 Compare June 1, 2026 23:29
Comment thread apps/prs/angular/src/routes/bugs/3762/bug3762.component.ts Outdated
Comment thread apps/prs/angular/src/routes/docs/app-header/app-header.component.ts Outdated
Comment thread libs/web-components/src/common/utils.ts
Comment thread libs/web-components/src/common/utils.ts Outdated
Comment thread libs/web-components/src/components/app-header/AppHeader.svelte Outdated
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from 887c958 to 762bac2 Compare June 2, 2026 22:52
Comment thread libs/web-components/src/common/utils.ts Outdated
@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch 2 times, most recently from b03ce14 to 501639e Compare June 3, 2026 23:09
@willcodeforcoffee

Copy link
Copy Markdown
Collaborator Author

Addressed your feedback @vanessatran-ddi , including:

  • fixed the docs differences
  • fixed the padding around buttons on Angular to match React
  • Renamed some variables and functions

I did not remove the slotName from AppHeaderNavigation because some users might still be using that. I don't think we want to break backward compatibility yet?

@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from bb0ed36 to d967608 Compare June 4, 2026 16:51
@willcodeforcoffee

Copy link
Copy Markdown
Collaborator Author

Updates:

  • deprecated the slotName attribute on AppHeaderMenu React and Angular components
  • added a demo of combining the two in the PRs pages

@vanessatran-ddi vanessatran-ddi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@vanessatran-ddi vanessatran-ddi requested a review from twjeffery June 5, 2026 00:30
"required": false,
"default": null,
"description": "V2 only: Secondary text displayed under the service name."
"description": "Secondary text displayed under the service name."

@vanessatran-ddi vanessatran-ddi Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run npm run build:prod local, and I see app-header-menu.json on my local has a change,

Image Image

I don't understand why this PR doesn't commit this app-header-menu.json but I can see the slotName property be removed from preview link: https://govalta.github.io/ui-components/pr-preview/pr-3909/components/app-header/#react

Image

Looks like even we don't commit, the script run magically somewhere? cc @twjeffery or @bdfranck

@willcodeforcoffee willcodeforcoffee force-pushed the eric/bug3762-AppHeader-slots branch from d967608 to e6c4f3e Compare June 5, 2026 15:18

@twjeffery twjeffery left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willcodeforcoffee the new props are a nice improvement over the raw slot attribute.

One thing I ran into while testing: if the header's content changes after it's first on screen, it breaks. The clearest case is signing in. Signed out shows a "Sign in" button, the person signs in, and you swap in their profile menu. When that swap happens, React goes fully blank (a removeChild error in the console), and Angular doesn't crash but the utilities just disappear, the Sign in goes and the profile never shows.

From what I can tell it's because the slot content gets moved up and the wrapper div removed when the header first loads, so the frameworks lose track of those nodes when they try to change them later. A header that's set up once and never changes is totally fine, which is everything the examples cover today. But anything that swaps header content on sign-in would hit this.

I noticed Copilot suggested putting the slot on each item instead of wrapping them, and you didn't want to go that way at the time. I tried it for this sign-in case though, and it's what stops the crash, the swap works fine and the profile shows up. Maybe worth another look now that we know it can crash? Happy to dig into it together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working P2 Priority 2 (should have): Improves completeness and reduces the post-launch support load.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AppHeader examples using slot property

4 participants