goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.1k stars 121 forks source link

refactor(select): decouple hlm from brn #253

Open alexciesielski opened 3 months ago

alexciesielski commented 3 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Which package are you modifying?

What is the current behavior?

Closes #252

What is the new behavior?

Decouple hlm from brn. There should be no mention of hlm inside brn. Otherwise it would be a logical circularity.

I think that brn in general should contain no components, otherwise it is taking away control over the final component library from developers.

IMHO, brn should be a set of directives and helpers to build up a component library - something along the lines of a cdk. I think that less logic in brn and more logic in hlm is a good thing as it gives developers the most control. But is also more error-prone in case of future updates. Not sure what the best course of action here is, alhtough I tend to think that more code in developer's codebases is a good thing (because it gives more control).

Anyway back to the PR:

What I'm unhappy about with the current solution is the fact that I had to make updateArrowDisplay an async function because it needs to access the viewport ElementRef, which is now passed to the directive through a signal.

Happy to hear suggestions for a cleaner solution for this.

In general I tried to leave as much code as possible inside the brn directives, although again I'm wondering how much UI-related logic should live inside brn (again, imho, none or as little as necessary - although I'm having trouble defining what necessary in this case means).

Also, I didn't touch the tests and generators yet. Once we're happy with the changes I will get to it, too.

Does this PR introduce a breaking change?

Other information

thatsamsonkid commented 3 months ago

Sorry, figured I would move this to the issue discussion, seemed more appropriate! #252