feat: dropdown menu polishing

[marcjulian]

PR Checklist

Please check if your PR fulfills the following requirements:

• [ ] The commit message follows our guidelines: https://github.com/spartan-ng/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [ ] Bugfix
• [x] Feature
• [ ] Code style update (formatting, local variables)
• [x] Refactoring (no functional changes, no api changes)
• [ ] Build related changes
• [ ] CI related changes
• [x] Documentation content changes
• [ ] Other... Please describe:

Which package are you modifying?

Primitives

• [ ] accordion
• [ ] alert
• [ ] alert-dialog
• [ ] aspect-ratio
• [ ] autocomplete
• [ ] avatar
• [ ] badge
• [ ] breadcrumb
• [ ] button
• [ ] button-group
• [ ] calendar
• [ ] card
• [ ] carousel
• [ ] checkbox
• [ ] collapsible
• [ ] combobox
• [ ] command
• [ ] context-menu
• [ ] data-table
• [ ] date-picker
• [ ] dialog
• [ ] empty
• [x] dropdown-menu
• [ ] field
• [ ] form-field
• [ ] hover-card
• [ ] icon
• [ ] input
• [ ] input-group
• [ ] input-otp
• [ ] item
• [ ] kbd
• [ ] label
• [x] menubar
• [ ] navigation-menu
• [ ] pagination
• [ ] popover
• [ ] progress
• [ ] radio-group
• [ ] resizable
• [ ] scroll-area
• [ ] select
• [ ] separator
• [ ] sheet
• [ ] sidebar
• [ ] skeleton
• [ ] slider
• [ ] sonner
• [ ] spinner
• [ ] switch
• [ ] table
• [ ] tabs
• [ ] textarea
• [ ] toggle
• [ ] toggle-group
• [ ] tooltip
• [ ] typography

Others

• [ ] trpc
• [ ] nx
• [ ] repo
• [ ] cli

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

• [ ] Yes
• [ ] No

Other information

[marcjulian]

Should I rename the package, imports and CLI command from menu to dropdown-menu @goetzrobin @ashley-hunter @MerlinMoos?

[marcjulian]

I am trying to add a HlmMenuTrigger directive, because BrnMenuTrigger exposes the input cdkMenuTriggerFor as brnMenuTriggerFor without public input I would need to add the input to BrnMenuTrigger

public readonly menuTriggerFor = input<CdkMenuTrigger['menuTemplateRef']>(undefined, { alias: 'brnMenuTriggerFor' });

Now when I create HlmMenuTrigger it works when I would use brnMenuTriggerFor but I cannot rename it again neither just with a new name nor with a new name and a public input. I would like to rename it to match the the directive selector hlmMenuTrigger. Than we could just use it as but the input renaming is not working.

<button hlmBtn variant="outline" [hlmMenuTrigger]="menu" align="start">Open</button>

imports: [HlmMenuImports, HlmButtonImports],

// input doesn't work - hlmMenuTriggerFor
<button hlmBtn variant="outline" hlmMenuTrigger [hlmMenuTriggerFor]="menu" align="start">Open</button>

// HlmMenuTrigger
import { CdkMenuTrigger } from '@angular/cdk/menu';
import { Directive, input } from '@angular/core';
import { BrnMenuTrigger } from '@spartan-ng/brain/menu';

@Directive({
	selector: '[hlmMenuTrigger]',
	hostDirectives: [
		{
			directive: BrnMenuTrigger,
			
			inputs: ['align', 'side', 'brnMenuTriggerFor: hlmMenuTriggerFor'],
		},
	],
	host: {
		'data-slot': 'dropdown-menu-trigger',
	},
})
export class HlmMenuTrigger {
	public readonly menuTriggerFor = input<CdkMenuTrigger['menuTemplateRef']>(undefined, { alias: 'hlmMenuTriggerFor' });
}

This works but than we still have the brnMenuTriggerFor

imports: [HlmMenuImports, HlmButtonImports],

// brnMenuTriggerFor input works
<button hlmBtn variant="outline" hlmMenuTrigger [brnMenuTriggerFor]="menu" align="start">Open</button>


import { Directive } from '@angular/core';
import { BrnMenuTrigger } from '@spartan-ng/brain/menu';

@Directive({
	selector: '[hlmMenuTrigger]',
	hostDirectives: [
		{
			directive: BrnMenuTrigger,
			inputs: ['align', 'side', 'brnMenuTriggerFor'],
		},
	],
	host: {
		'data-slot': 'dropdown-menu-trigger',
	},
})
export class HlmMenuTrigger {}

Any idea what could help here?

[MerlinMoos]

Hey from my point of view yes, shadcn is having the same naming conventions.

[marcjulian]

@MerlinMoos shadcn has also 3 different packages context-menu, dropdown-menu and menubar. As they share styles and logic anyway I guess its fine to only have one.

Sure I can rename it to dropdown-menu for consistency

[MerlinMoos]

Yes we can rename them and implement the other onces as a feature

[marcjulian]

Sure will rename it, we already have context-menu and menubar no need to add as a feature 👍 just for clarification

[marcjulian]

The package "menu" contains also the directives/components used for the HlmMenuBar.

Do I rename everything to HlmDropdownMenu and HlmDropdownMenubar or do I keep the name for Menubar specific just as HlmMenuBar (or maybe HlmMenubar)?

• package menu to dropdown-menu - brain and helm
• update imports @spartan-ng/brain/menu to '@spartan-ng/brain/dropdown-menu' - same for helm
• Rename HlmMenu to HlmDropdownMenu
• Rename HlmMenuBar to HlmMenubar or add Dropdown too? Or maybe move them into menubar package?

I also noticed that the Brain menu used CDK Menu under the hood, which basically acts as the "brain" for the menu components/directives. Is it necessary to provide @spartan-ng/brain/menu as a wrapper around the CDK Menu, which could already be the "brain" part in my opinion.

Some brain menu directives are just wrapping the CDK Menu, like BrnMenuBar. Maybe in some cases it may make sense if we need additional logic. But we don't have an additional spartan brain package for carousel or sonner which providertheir own "brain"/logic part.

import { CdkMenuBar } from '@angular/cdk/menu';
import { Directive } from '@angular/core';

@Directive({
	selector: '[brnMenuBar]',
	hostDirectives: [CdkMenuBar],
})
export class BrnMenuBar {}

@goetzrobin @ashley-hunter @MerlinMoos what are your thoughts?

[ashley-hunter]

I think move menu bar to its own package and keep that name as is, matches the shadcn naming.

if we don't have any logic on top of the cdk implementation I don't think we need a brain directive, it's just adding unnecessary bloat, and like you say, we don't do it for others.

[MerlinMoos]

Im on ashleys side. We should keep our brain slim with unnecessary directives. For the naming we should move it to our own package

[marcjulian]

Sounds good to me, I think this would also resolve stuff around the HlmMenuTrigger (https://github.com/spartan-ng/spartan/pull/1017#issuecomment-3523093580).

I guess menubar can depend on dropdown-menu for specific dropdown stuff, so we don't have to duplicate things either.

[marcjulian]

Current todos

• [ ] Add healthcheck and migration for dropdown-menu, menubar and context-menu
• [ ] Migrate example and storybook to new helm packages
• [ ] Remove helm package "menu"
• [ ] Remove brain package "menu"
• [x] Move brn-menu-align to core