feat: add tooltip component - #36
Conversation
|
Coverage report for commit: 71e8360 Summary - Lines: - | Methods: -
🤖 comment via lucassabreu/comment-coverage-clover |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR adds a new Blade tooltip component to the brave component set, intended to be used together with JavaScript from brave-frontend-kit.
Changes:
- Register a new
Tooltipview component inComponentsServiceProvider. - Add the
Tooltipcomponent class. - Add Blade views for the tooltip container, trigger, and arrow subcomponents.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ComponentsServiceProvider.php | Registers the new Tooltip component for Blade usage under the brave prefix. |
| src/Components/Tooltip.php | Adds a class-based Blade component that renders the tooltip view. |
| resources/views/components/tooltip/trigger.blade.php | Adds a trigger <button> component for tooltips. |
| resources/views/components/tooltip/index.blade.php | Adds the tooltip container markup and default attributes. |
| resources/views/components/tooltip/arrow.blade.php | Adds a small arrow subcomponent for tooltip styling/positioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @props(['id', 'ariaDescribedby']) | ||
|
|
||
| <button | ||
| {{ $attributes->merge([ | ||
| 'id' => $id, | ||
| 'type' => 'button', | ||
| 'class' => 'js-brave-tooltip-trigger', | ||
| 'aria-describedby' => $ariaDescribedby, |
There was a problem hiding this comment.
The prop name ariaDescribedby is inconsistent with the camelCase style used elsewhere (e.g. ariaLabel in other components). Consider renaming it to ariaDescribedBy (and update the merge mapping) so multi-word props follow the same casing and are easier to use consistently from Blade.
| @props(['id', 'ariaDescribedby']) | |
| <button | |
| {{ $attributes->merge([ | |
| 'id' => $id, | |
| 'type' => 'button', | |
| 'class' => 'js-brave-tooltip-trigger', | |
| 'aria-describedby' => $ariaDescribedby, | |
| @props(['id', 'ariaDescribedBy']) | |
| <button | |
| {{ $attributes->merge([ | |
| 'id' => $id, | |
| 'type' => 'button', | |
| 'class' => 'js-brave-tooltip-trigger', | |
| 'aria-describedby' => $ariaDescribedBy, |
There was a problem hiding this comment.
Het is ook aria-describedby en niet aria-described-by dus ik vond het eigenlijk wel consistent zo.
| {{ $attributes->merge([ | ||
| 'id' => $id, | ||
| 'role' => 'tooltip', | ||
| 'aria-hidden' => 'true', |
There was a problem hiding this comment.
aria-hidden defaults to true on the tooltip element, but this element is intended to be referenced via aria-describedby from the trigger. Content referenced by ARIA relationships should not be hidden from assistive technologies, otherwise many screen readers will ignore the description. Consider removing the hard-coded aria-hidden default (or default it to false and let JS/consumers toggle it when the tooltip is actually hidden/visible).
| 'aria-hidden' => 'true', |
There was a problem hiding this comment.
Hmmmm, @YvetteNikolov wat vind jij? Ik zou denken dat een tooltip altijd standaard verborgen is. Misschien dan de hidden class default toevoegen?
There was a problem hiding this comment.
Ik vind dit genitpick door copilot. En ik ben geen fan van Tailwind classes toevoegen, hou het liever framework agnostisch. Van mij mag je deze comment negeren.
2e5bd64 to
e7f1ab0
Compare
YvetteNikolov
left a comment
There was a problem hiding this comment.
Super! Kun je in de README nog een simpel voorbeeld toevoegen?
e7f1ab0 to
71e8360
Compare
Samen met javascript in brave-frontend-kit yardinternet/brave-frontend-kit#47.
Een voorbeeld hoe ik het nu heb toegepast: