-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add tooltip component #36
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| @props(['id', 'arrowClass' => '']) | ||
|
|
||
| <div | ||
| {{ $attributes->merge([ | ||
| 'id' => $id, | ||
| 'role' => 'tooltip', | ||
| 'aria-hidden' => 'true', | ||
| 'class' => 'js-brave-tooltip', | ||
| ]) }}> | ||
| {{ $slot }} | ||
| <div @class(['js-brave-tooltip-arrow', $arrowClass])></div> | ||
| </div> | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||||||||||||||||||||||||||||||
| @props(['id', 'ariaDescribedby']) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| <button | ||||||||||||||||||||||||||||||||||
| {{ $attributes->merge([ | ||||||||||||||||||||||||||||||||||
| 'id' => $id, | ||||||||||||||||||||||||||||||||||
| 'type' => 'button', | ||||||||||||||||||||||||||||||||||
| 'class' => 'js-brave-tooltip-trigger', | ||||||||||||||||||||||||||||||||||
| 'aria-describedby' => $ariaDescribedby, | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+8
|
||||||||||||||||||||||||||||||||||
| @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Het is ook aria-describedby en niet aria-described-by dus ik vond het eigenlijk wel consistent zo.
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.
Hehe prima zo
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Yard\Brave\Components; | ||
|
|
||
| use Illuminate\Contracts\View\Factory; | ||
| use Illuminate\View\Component; | ||
| use Illuminate\View\View; | ||
|
|
||
| class Tooltip extends Component | ||
| { | ||
| public function render(): Factory|View | ||
| { | ||
| return view('brave::components.tooltip.index'); | ||
| } | ||
| } |
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.
aria-hiddendefaults totrueon the tooltip element, but this element is intended to be referenced viaaria-describedbyfrom 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-codedaria-hiddendefault (or default it tofalseand let JS/consumers toggle it when the tooltip is actually hidden/visible).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.
Hmmmm, @YvetteNikolov wat vind jij? Ik zou denken dat een tooltip altijd standaard verborgen is. Misschien dan de
hiddenclass default toevoegen?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.
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.