spartan icon indicating copy to clipboard operation
spartan copied to clipboard

fix: Add support for wrapped form controls

Open dolanmiu opened this issue 3 weeks ago • 3 comments

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)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] 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
  • [ ] dropdown-menu
  • [ ] field
  • [x] form-field
  • [ ] hover-card
  • [ ] icon
  • [ ] input
  • [ ] input-group
  • [ ] input-otp
  • [ ] item
  • [ ] kbd
  • [ ] label
  • [ ] 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?

The hlm-form-field component only supports BrnFormFieldControl (descendant controls). It requires an explicit control directive to be present and doesn't work with wrapped BrnFormFieldControls or standard Angular reactive forms using formControl or formControlName directives.

image

I usually wrap things into components for re-usability:

@Component({
  selector: 'app-select',
  imports: [ReactiveFormsModule, HlmSelectImports, BrnSelectImports],
  template: `
    <hlm-select class="inline-block w-full" placeholder="Select values" [formControl]="formControl">
      <hlm-select-trigger class="w-full">
        <hlm-select-value />
      </hlm-select-trigger>
      <hlm-select-content class="max-h-96">
        <hlm-select-scroll-up />
        @for (obj of getSortedOptions(items()); track obj.item; let i = $index) {
          <hlm-option [value]="obj.item">{{ obj.label }}</hlm-option>
        }
        <hlm-select-scroll-down />
      </hlm-select-content>
    </hlm-select>
  `,
  providers: [
    {
      provide: NG_VALUE_ACCESSOR,
      useExisting: forwardRef(() => Select),
      multi: true,
    },
  ],
  changeDetection: ChangeDetectionStrategy.OnPush,
})
export class Select<T> implements ControlValueAccessor {
  ...
}
            <hlm-form-field>
              <label hlmLabel for="input-{{ pair.key }}"
                ><span hlmBadge variant="default">{{ pair.key }}</span></label
              >
                <app-select
                  [items]="toOptions(secrets()[pair.key].options)"
                  [formControl]="pair.value"
                  id="input-{{ pair.key }}"
                />
            </hlm-form-field>

Currently, HlmFormField doesn't work with this.

Closes #

What is the new behavior?

The hlm-form-field component now supports both:

  • BrnFormFieldControl (existing behavior)
  • Wrapped BrnFormFieldControls
  • Angular NgControl (formControl/formControlName) directives

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

This enhancement improves flexibility by allowing developers to use standard Angular reactive forms patterns with the form-field component, without requiring the BrnFormFieldControl directive. This means BrnFormFieldControls wrapped in a component.

dolanmiu avatar Nov 12 '25 18:11 dolanmiu

Hey @dolanmiu!

Thanks for your PR!

The Form primitives are currently one of the part of the project that we definitely want to improve before releasing v1.

Here is another PR that tries tackling this: https://github.com/spartan-ng/spartan/pull/911 There's also been some discussion in our discord here: https://discord.com/channels/1145362148621557921/1145362149133271082/1437880161474187314

One thing that's also relevant to this PR and might need some better instructions in the contribution guide: I only see your changes in the .template files - these are actually auto generated through the following generator: nx g @spartan-ng/tools:hlm-to-cli-generator

The changes are actually coming from the libs/helm project.

Please let me know if this is helpful!

goetzrobin avatar Nov 12 '25 19:11 goetzrobin

Thanks for the quick reply! Good to know it's being worked on. Can close this then?

dolanmiu avatar Nov 12 '25 22:11 dolanmiu

@dolanmiu let's keep this for now as a reference, because I absolutely believe this should be supported!

goetzrobin avatar Nov 13 '25 11:11 goetzrobin