refactor(checkbox): new helm api

elite-benni commented 7 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

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

PR Type

What kind of change does this PR introduce?

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

Which package are you modifying?

[ ] accordion
[ ] alert
[ ] alert-dialog
[ ] aspect-ratio
[ ] avatar
[ ] badge
[ ] button
[ ] calendar
[ ] card
[x] checkbox
[ ] collapsible
[ ] combobox
[ ] command
[ ] context-menu
[ ] data-table
[ ] date-picker
[ ] dialog
[ ] dropdown-menu
[ ] hover-card
[ ] input
[ ] label
[ ] menubar
[ ] navigation-menu
[ ] popover
[ ] progress
[ ] radio-group
[ ] scroll-area
[ ] select
[ ] separator
[ ] sheet
[ ] skeleton
[ ] slider
[ ] switch
[ ] table
[ ] tabs
[ ] textarea
[ ] toast
[ ] toggle
[ ] tooltip
[ ] typography

What is the current behavior?

The old api was very verbose:

<brn-checkbox class="mr-2" hlm>
  <hlm-checkbox-checkicon />
</brn-checkbox>

Closes #

What is the new behavior?

The new API for helm is simpler

Does this PR introduce a breaking change?

[x] Yes
[ ] No

The Checkbox with the old helm API will stop working. You will have to replace

<brn-checkbox class="mr-2" hlm>
  <hlm-checkbox-checkicon />
</brn-checkbox>

with

<hlm-checkbox class="mr-2" />

Other information

ajitzero commented 7 months ago

Can we still retain an optional projection slot to replace the icon?

Instead of:

<hlm-checkbox>
  <hlm-checkbox-checkicon />
  <!-- OR -->
  <hlm-icon hlmAlertIcon name="radixStarFilled" />
</hlm-checkbox>

Something like:

<hlm-checkbox />

goetzrobin commented 7 months ago

Can we still retain an optional projection slot to replace the icon?

Instead of:
<hlm-checkbox>
  <hlm-checkbox-checkicon />
  
  <hlm-icon hlmAlertIcon name="radixStarFilled" />
</hlm-checkbox>
Something like:
<hlm-checkbox />

You'll always own the code for hlm so you can change the icon in your code. But I am not sure if that is what you're asking or if you're saying you'd like to override it in specific use cases?

ajitzero commented 7 months ago

you'd like to override it in specific use cases?

I was thinking about making on-off cases where the icon could be "Bookmark" icon or a star-based rating icon, but that's probably better off as a custom switch or a toggle button, not a checkbox.

Please disregard this.

goetzrobin commented 7 months ago

you'd like to override it in specific use cases?

I was thinking about making on-off cases where the icon could be "Bookmark" icon or a star-based rating icon, but that's probably better off as a custom switch or a toggle button, not a checkbox.

Please disregard this.

No that's actually valid! I'd say that's a custom use case where you could create an input for the icon name maybe? 🤔

ajitzero commented 7 months ago

you could create an input for the icon name

Yes, that would be the way to go. I wanted to make as few logical modifications as possible and limit my copy of the file to styling (Tailwind) changes only - mainly to reduce conflicts while fetching newer changes from this repo.

elite-benni commented 7 months ago

you could create an input for the icon name

Yes, that would be the way to go. I wanted to make as few logical modifications as possible and limit my copy of the file to styling (Tailwind) changes only - mainly to reduce conflicts while fetching newer changes from this repo.

Will add an input for the icon name.

goetzrobin commented 7 months ago

@elite-benni let’s not add it and leave it up to the owners to add that. Simple because you’ll also have to provide the icon you want to use in the providers array. We can’t know which ones they’d pick

elite-benni commented 7 months ago

oops just pushed it. the user has to provide it in the parent. Do you think that is bad?

Looks like this:

@Component({
    selector: 'spartan-checkbox-own-icon',
    standalone: true,
    imports: [HlmLabelDirective, HlmCheckboxComponent],
    providers: [provideIcons({ radixText })],
    template: `
        <label class="flex items-center" hlmLabel>
            <hlm-checkbox class="mr-2" checkIconName="radixText" />
            Accept terms and conditions
        </label>
    `,
})
export class CheckboxOwnIconComponent {}

goetzrobin / spartan