-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Renaming the generated hlm-
selector to anything else breaks the select
component
#252
Comments
Let's do this. You're absolutely correct that brain shouldn't functionally depend on hlm. I like your proposed solution and that should also allow us to use the new APIs in 17.3 if you want. |
As discussed in Discord my initial proposal is not as elegant as using Which opens another can of worms: How much template/UI-logic should even exist inside The more I think about it, the more I come to the conclusion that ideally Also tagging @thatsamsonkid and @elite-benni as you guys are also deeply involved in the library. |
Although if I continue that thought brn would be nothing more than a cdk with some code scaffolding. I guess my confusion stems from the lack of a clear idea in my mind of what brn is supposed to be... I will continue converting the brn select components to directives for now, moving the templates to hlm, and once that is materialized then I guess the picture should become clearer. |
I will be honest this is a lot more changes than I was expecting. So overall this kind of transition is a very slippery slope for us from a library maintenance perspective. To be clear I'm not against your objective here which is, from the helm perspective be able to make all of the customizations you could ever want to it and it still continue to work properly, especially in regards to selector renaming for example. However I don't believe this is the way for the reasons below. (Apologize for the long read and also this is only my view, I don't wish to speak for @goetzrobin and @elite-benni ) The UI Library Paradigm (as I understand it at least)Brn: is meant to still be out of the box a functioning component, unstyled but functioning and not just a set of pieces left to the developer to figure out how to use to make a working component, even if documented. Hlm: is meant to be low logic and primarily focused on applying the shadcn skin and not much else. Why Hlm has low logic and should continue this way:Hlm is in the full control of the end user, moving all the logic there while sure gives great customizability to the end user also has a lot of downsides in terms of maintainability and upgradability of this library.
Why helm selectors were placed in select brn:The decision for having the helm selectors in the brn component was not a design decision that was taken lightly. I ran into very much the same issues you are running into yourself. Most alternative solutions ultimately resulted in a much more verbose API. Ultimately we are limited in what Angular allows us to do but our ultimate goal was to provide the best API possible and having the helm selectors in brn, while not ideal, is ultimately not a hard dependency and seemed reasonable compared to the other options. There is currently no hard requirement for hlm to be present at all for brn component to function. Closing wordsNow I'm not saying there is no solution for customizing the helm selector, I feel like there should be a way for us to provide a slightly different alternative if there really is a desire to customize the helm selector, but ultimately I believe we should support both because that alternative will definitely be a much more verbose API and IMHO I feel greater amount of people would prefer a simpler API over the ability to customize the selector, which is the only downside with current implementation I see. Again I really want to support the helm selector customization, I really hate that the select component right now is the only component in the library that seems to have this shortcoming at the moment. |
Thanks for the write-up. I think it is important to clarify these assumptions and expectations and define them in a central place, probably best in the Intro/Getting-started section of the documentation, so these questions don't keep popping up in the future, as I'm sure I'm not the only one looking for a definition of what brn, and what hlm really is supposed to be. For a bit of context of what my motivations are: in a customer's project we are using Angular Material components for their superb API, there is no other component library which has such a good and well though out API. Still, the fact that it is so opinionated, not only from a styling perspective, but also from an extensibility perspective caused us to write a lot of additional code to "make it ours". I really like the idea of shadcn, which is why I'll copy the part I most liked from it's Introduction docs:
This clicked with me most importantly because of this part: The code is yours. I want for our project to have full control over the library that we will be building, and my vision for such a library is to have as great an API as Material has without compromises. Meaning that while looking at the refactoring mentioned above I backtracked from my initial idea of defining additional directives just for We both agree that it's unnecessarily more verbose when I get the idea of upgradibility and maintainability and that it will be a pain to apply upgrades to the generated code files - this was a big question mark for me when reading more about shadcn. How would this be solved in a somewhat automatic manner? But if the decision has to be made between either full control of code, or easier upgrading in the future, I vote full control. Once the component library stands and is in use and is proven to work there is little reason to make additional changes to it, other than applying bugfixes and/or adding accessibility features. Which is why I claim that upgradibility is less important (to me). So I guess the final question becomes: what is spartan supposed to be?
I don't see how it's possible to marry the ideas of Edit: another thought that came to mind in regards to your statement
But that's exactly the beauty of the code scaffolding commands - there isn't really anything to figure out for anybody if they want to use the components differently than anticipated by spartan. And as a lighthearted end-note - if we're to take the naming convention seriously, then I wouldn't want my brain to be exposed uncovered to the outside world - I would prefer having a helm(et) in between. Which is to say the brains are gonna be wrapped in a helmet library anyway, so why not separate the looks from the brain completely? |
So this is a tough one. I don't consider it a hard dependency on helm. It's more of a quality of life improvement for those using the hlm scaffolded components. I just read this article: https://angular.io/guide/content-projection We can use the ngProjectAs attribute to allow for custom components. We are absolutely limited by Angular here. I tried using the host property inside the Angular @component to apply the ngProjectAs, but it did not work. I do believe that it should though: We wouldn't use a dynamic attribute, but a static one. Here is more info on this issue too. Here is an example of it working with ngProjectAs export const CustomTrigger: Story = {
render: (args) => ({
props: { ...args },
moduleMetadata: {
imports: [CustomSelectTriggerComponent],
},
template: /* HTML */ `
<hlm-select class="inline-block" ${argsToTemplate(args, { exclude: ['initialValue'] })}>
<custom-select-trigger ngProjectAs="[brnSelectTrigger]" class="w-56">
<hlm-select-value />
</custom-select-trigger>
<hlm-select-content>
<hlm-select-label>Fruits</hlm-select-label>
<hlm-option value="apple">Apple</hlm-option>
<hlm-option value="banana">Banana</hlm-option>
<hlm-option value="blueberry">Blueberry</hlm-option>
<hlm-option value="grapes">Grapes</hlm-option>
<hlm-option value="pineapple">Pineapple</hlm-option>
</hlm-select-content>
</hlm-select>
`,
}),
};
@Component({
selector: 'custom-select-trigger',
standalone: true,
imports: [BrnSelectTriggerDirective, HlmIconComponent],
providers: [provideIcons({ lucideChevronDown })],
template: `
<button [class]="_computedClass()" #button brnSelectTrigger type="button">
<ng-content />
@if (icon) {
<ng-content select="hlm-icon" />
} @else {
<hlm-icon class="ml-2 h-4 w-4 flex-none" name="lucideChevronDown" />
}
</button>
`,
})
export class CustomSelectTriggerComponent {
@ViewChild('button', { static: true })
public buttonEl!: ElementRef;
@ContentChild(HlmIconComponent, { static: false })
protected icon!: HlmIconComponent;
public readonly userClass = input<ClassValue>('', { alias: 'class' });
protected readonly _computedClass = computed(() =>
hlm(
'!bg-sky-500 flex h-10 items-center justify-between rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background placeholder:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 w-[180px]',
this.userClass(),
),
);
} What are your thoughts? I think if using ngProjectAs would work if applied as a static host field this would resolve our issue. Do you think it's worth opening an issue with the Angular team? |
I think it's worth opening an issue. The reproduction is very simple and I have played around with it and to a developer there is no reason it should not work with the static host attribute. Maybe the Angular team has other ideas of working around the issue. What do you think? |
I think it's worth opening, I think maybe they could give us an idea of maybe an alternative or workaround and in general I think this could be useful for us in some other places as well in the library if resolved |
When you open it, can you link it with this issue for reference please? |
I opened one: angular/angular#55438 I forgot to link it. But will add it now |
Please provide the environment you discovered this bug in.
Any environment with
select
Which area/package is the issue in?
select
Description
As already mentioned in Discord I like to make use of the fact that I have full control over the generated components in my repository and like to change the
hlm
prefix to my customer's prefixes.Unfortunately this breaks the
select
component because thebrn
part specifically selects for both alabel[hlmLabel]
andhlm-select-trigger
here.I argue that this approach is architecturally and conceptually incorrect, because it's not only a (conceptually) circular dependency, but also tightly couples the
hlm
implementation to thebrn
part.While, if I understand correctly,
brn
is supposed to be a headless standalone collection of directives and components and should be unaware of where/how it's used.Proposed solution
Again, as already mentioned in Discord, my proposed solution would be to decouple
hlm
frombrn
and instead of usingng-content
with aselect
I would instead opt for the (while more verbose) more explicit way of passing a template reference either through an@Input
or (as proposed in Discord) through a@ContentChild
.Since we're still in alpha I would even suggest taking the dive and making this a breaking change and completely removing any mention of
hlm
insidebrn
, although I think it should be possible to keep the current solution while adding support for selecting the label and trigger throughContentChild
.I would even go as far as suggesting adding a eslint rule for every
brn
library that would consist of a regex looking forhlm
throughout its code and erroring on that.Please provide the exception or error you saw
No response
Other information
Happy to open a PR with the suggested solution of tagging the (hlm)
label
andtrigger
components with a directive and querying it in theBrnSelect
component using@ContentChild
I would be willing to submit a PR to fix this issue
The text was updated successfully, but these errors were encountered: