refactor(tooltip): migrate to signals #430 #503
Conversation
|
@elite-benni @goetzrobin I changed the naming as discussed in the other PR here as well |
There was a problem hiding this comment.
I don't like using the mutable + state pattern when it is not needed.
I think providing the defaultOptions via Injector like it is done with the HlmButtonDirective, is more readable and declarative.
I added a PR to your branch where I added some of the changes and we can talk about it.
There was a problem hiding this comment.
I left some comments. I think it's just some minor naming stuff and one more philosophical question about introducing a new helper in core when we use it in only a single spot. @elite-benni I trust your review on this as you've already spent some time looking at this code also!
Thanks so much for both of your guys work on this!
| // Must wait for the template to be painted to the tooltip so that the overlay can properly | ||
| // calculate the correct positioning based on the size of the tek-pate. | ||
| if (this._tooltipInstance) { | ||
| this._tooltipInstance.content = this.content; | ||
| this._tooltipInstance.content = this.brnTooltipTriggerState(); |
There was a problem hiding this comment.
Same here. And why does it replace this.content?
There was a problem hiding this comment.
hey, content had an alias which was brnTooltipTrigger, I will refactor the naming
PR Checklist
Please check if your PR fulfills the following requirements:
guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
PR Type
What kind of change does this PR introduce?
Which package are you modifying?
What is the current behavior?
Closes #430
What is the new behavior?
Does this PR introduce a breaking change?
Other information