-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat(tabs): exposes new values in brn-tabs-trigger #104
feat(tabs): exposes new values in brn-tabs-trigger #104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple comments. Curious to hear your thoughts
@@ -48,4 +48,12 @@ export class BrnTabsTriggerDirective { | |||
if (!this._key) return; | |||
this._root.setValue(this._key); | |||
} | |||
|
|||
get isSelected(): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe instead change it to a signal called _selected and expose a public readonly selected that is _selected.asReadonly()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goetzrobin - I was just seeing computed signals are readonly by default. So could we not just do the following changes. See this PR thatsamsonkid@8b46768
Let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! Much cleaner! Tabs was one of the earlier components so I am glad we are revisiting this and cleaning things up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I went ahead merged those changes in
Also, @thatsamsonkid can you add the exposedAs for this issue: #105? Thanks already! |
1 similar comment
Also, @thatsamsonkid can you add the exposedAs for this issue: #105? Thanks already! |
All sounds like good ideas to me, I'll make the changes later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
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?
Brn Trigger currently does not expose the underlying elementRef as well as some other key values that could be useful in customizing the tabs component.
See this as an example of code used to create below tabs example
https://github.com/thatsamsonkid/portfolio-analog/blob/main/src/app/content/experience.component.ts#L199
Closes #
What is the new behavior?
PR update Brn trigger directive to expose underlying elementRef as well getters for key and if the tab is currently active.
Does this PR introduce a breaking change?
Other information
Screen.Recording.2024-01-03.at.12.40.18.PM.mov