Skip to content

Feat/navbar vertical next inline collapse active item v2#2109

Draft
mistrykaran91 wants to merge 5 commits into
feat/navbar-vertical-next-inline-collapse-portalfrom
feat/navbar-vertical-next-inline-collapse-active-item-v2
Draft

Feat/navbar vertical next inline collapse active item v2#2109
mistrykaran91 wants to merge 5 commits into
feat/navbar-vertical-next-inline-collapse-portalfrom
feat/navbar-vertical-next-inline-collapse-active-item-v2

Conversation

@mistrykaran91
Copy link
Copy Markdown
Member

@mistrykaran91 mistrykaran91 commented May 29, 2026

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an inline collapse feature for the vertical navigation bar, displaying the active navigation item as a chip next to the toggle button when collapsed. Feedback on these changes highlights two issues: first, when the navbar is expanded, the item label element relocated by DomPortal becomes orphaned and disappears, which can be resolved by manually appending it back to its host element in a browser-safe effect; second, the enter animation transition for the active item chip will not trigger because no transition is active during the enter phase, which can be fixed by defining the transition directly on the active item button class.

* template to relocate the label into the inline-collapse chip button.
* @internal
*/
protected readonly labelPortalRef = computed(() => new DomPortal(this.labelContentEl()));
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.

high

When the navbar is collapsed, the DomPortal relocates the #labelContent element from the item's DOM into the active item chip. However, when the navbar is expanded again and the portal is detached, DomPortal does not automatically restore the element to its original parent. This causes the label to become orphaned and completely disappear from the expanded navbar item.

To fix this, we should manually append the element back to the component's host element when the item is no longer active as a root item. Since this involves direct DOM manipulation and Angular CDK features, we must ensure it only runs in a browser environment using isPlatformBrowser to prevent Server-Side Rendering (SSR) errors.

This can be done cleanly using an effect in the constructor:

  private readonly elementRef = inject(ElementRef);
  private readonly platformId = inject(PLATFORM_ID);

  constructor() {
    effect(() => {
      if (isPlatformBrowser(this.platformId) && !this.isActiveRootItem()) {
        const el = this.labelContentEl()?.nativeElement;
        if (el && el.parentNode !== this.elementRef.nativeElement) {
          this.elementRef.nativeElement.appendChild(el);
        }
      }
    });
  }
References
  1. When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).

Comment on lines +197 to +206
&.fade-enter {
@starting-style {
opacity: 0;
}
}

&.fade-leave {
opacity: 0;
transition: opacity all-variables.element-transition-duration(0.15s) linear;
}
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.

medium

The @starting-style transition for the enter animation (fade-enter) will not work because there is no transition property active during the enter phase. For @starting-style to animate, a transition must be defined on the element itself or during the enter state.

We can define the transition directly on .active-item-button. This also allows us to simplify fade-leave by inheriting the transition, avoiding duplication and ensuring that the leave animation is smooth and consistent.

      transition: opacity all-variables.element-transition-duration(0.15s) linear;

      &.fade-enter {
        @starting-style {
          opacity: 0;
        }
      }

      &.fade-leave {
        opacity: 0;
      }
References
  1. Ensure animations for elements leaving the view are smooth (e.g., fade-out) and consistent with other animations, rather than being instant.

@mistrykaran91 mistrykaran91 force-pushed the feat/navbar-vertical-next-inline-collapse-active-item-v2 branch from c111318 to 390bb1c Compare May 29, 2026 08:44
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.

1 participant