feat(tooltip): support passing `string` to tooltip trigger

goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.

https://spartan.ng

MIT License

1.1k stars 121 forks source link

feat(tooltip): support passing `string` to tooltip trigger #232

Closed alexciesielski closed 3 months ago

alexciesielski commented 3 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[x] The commit message follows our guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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
[ ] icon
[ ] input
[ ] label
[ ] menubar
[ ] navigation-menu
[ ] pagination
[ ] popover
[ ] progress
[ ] radio-group
[ ] scroll-area
[ ] select
[ ] separator
[ ] sheet
[ ] skeleton
[ ] slider
[ ] sonner
[ ] spinner
[ ] switch
[ ] table
[ ] tabs
[ ] textarea
[ ] toast
[ ] toggle
[x] tooltip
[ ] typography

What is the current behavior?

Closes #222

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

goetzrobin commented 3 months ago

Looks great @alexciesielski! One idea. If we pass in a string should we set the aria described by to this string if there's no explicit one set? I'd also like to show a warning in dev mode if there's no aria label set, to let developers know they should set one to help users that use accessibility tech. Thoughts?

alexciesielski commented 3 months ago

Hey,

I added a brnDevMode flag because I didn't find one yet, it's based on ngDevMode which gets set by the Angular compiler to (obviously) false when it is run with production mode enabled.

This then allows the compiler to trees-hake any code that it encounters, where the flag was used, which now basically looks like:

if(false) {
 // dev code that should not go into prod
}

and I used it to show a warning if the describedby input wasn't set. But I guess it would be more elegant to just set it for the user. How would you suggest I go about this?

Maybe make an effect(() => ) in the constructor and set it to the tooltipTrigger if it is a string and the aria-describedby isn't set?

elite-benni commented 3 months ago

I Really like this addition! Good Job! ;)