Skip to content
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

Refactor: Replace @HostBinding with host property of Component Decorator #58

Merged
merged 16 commits into from
Nov 21, 2023

Conversation

elite-benni
Copy link
Collaborator

@elite-benni elite-benni commented Nov 20, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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?

  • accordion
  • alert
  • alert-dialog
  • aspect-ratio
  • avatar
  • badge
  • button
  • calendar
  • card
  • 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 Binding of the Class is set to Host with a @HostBinding, which is not the preverred approach as stated in angular.dev.
@HostBinding also does not work with onPush Parent Components and changes to the property.

Closes #

What is the new behavior?

The classes are bound to the host property of the component decorator.
Signals or computed that are bound via host property also work with changes.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I also implemented the userCls signal in every Component with the same structure, so we should not have a too hard time when migrating to signal based inputs.
closes #55

Copy link

vercel bot commented Nov 20, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @goetzrobin on Vercel.

@goetzrobin first needs to authorize it.

@goetzrobin
Copy link
Collaborator

Looks good to me! There is a tiny conflict after merging a different PR, once you fix that one I will merge it!

@goetzrobin
Copy link
Collaborator

goetzrobin commented Nov 20, 2023

@elite-benni Thank you! That was fast!
Noticed another minor thing: Can we make all the signals readonly. Just to add a layer of "security" as signals are not meant to be reassigned. Another mess I am trying to clean up lol

Copy link
Contributor

@nartc nartc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Though I have a non-blocking question, some places still use generateClasses() private method while some other places don't. Can we make this more consistent?

@elite-benni
Copy link
Collaborator Author

Sure, I can do this.
The generateClasses() that are left are because it was easier to replace the old code.

The whole idea of this change was to make it more consistent. Thanks for pointing that out.
I think keeping generateClasses everywhere, is easier to read.

I will add readonly to the signals.

@goetzrobin
Copy link
Collaborator

Btw you can run yarn prettify to format the files for you before committing since you are touching a good amount of them with this PR :)

@elite-benni
Copy link
Collaborator Author

I merged the conflict in github. ;) thats why i missed the empty line ;)

@goetzrobin goetzrobin merged commit 84476e9 into spartan-ng:main Nov 21, 2023
6 checks passed
@goetzrobin
Copy link
Collaborator

🚀 Thanks for making those changes!

@elite-benni elite-benni deleted the iss55 branch November 21, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Hostbinding to host property in the @Component decorator
3 participants