-
Notifications
You must be signed in to change notification settings - Fork 162
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: adding tooltips and tests to visual-refresh-toolbar #2650
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2650 +/- ##
==========================================
+ Coverage 95.90% 95.91% +0.01%
==========================================
Files 748 748
Lines 20732 20814 +82
Branches 7059 6720 -339
==========================================
+ Hits 19882 19963 +81
- Misses 794 843 +49
+ Partials 56 8 -48 ☔ View full report in Codecov by Sentry. |
ce09c55
to
3ca3174
Compare
3ca3174
to
e040c49
Compare
e040c49
to
d016767
Compare
d016767
to
e41eb5a
Compare
e41eb5a
to
04307bf
Compare
04307bf
to
fd038bc
Compare
dc5256b
to
a9429f9
Compare
*/ | ||
const handleOnFocus = useCallback( | ||
(event: FocusEvent) => { | ||
let shouldShowTooltip = false; |
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.
Minor: no need to declare a let
, can be declared as const
as it is assigned only once
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.
It gets modified though in the conditional below it
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.
But it's the only place where it is assigned, so it could be declared just there. Anyway not blocking.
const relatedTarget = eventWithRelatedTarget?.relatedTarget; | ||
const isFromAnotherTrigger = relatedTarget?.dataset?.shiftFocus === 'awsui-layout-drawer-trigger'; | ||
if ( | ||
isForSplitPanel || //for tab/key navigation to button from breadcrume |
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.
Nitpick: typo in comment (breadcrume
)
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.
Is it ever possible that isForSplitPanel
and isForPreviousActiveDrawer
are true at the same time? If not, there is no need to check for isForSplitPanel
in this condition.
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.
yes. when the split panel is open that both would be true
This adds a visual tooltip to the toolbar on visual-refresh-toolbar variant by adding event listeners to the toolbar-trigger-wrapper elements that shows the tooltip when focused or hovered.
Description
The trigger-button takes new props to determine if a tooltip should be possible.
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.