Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Tooltip]: adds arrow on tooltip default mode #457

Merged
merged 1 commit into from
Oct 31, 2023
Merged

[Tooltip]: adds arrow on tooltip default mode #457

merged 1 commit into from
Oct 31, 2023

Conversation

benjaminknox
Copy link
Contributor

@benjaminknox benjaminknox commented Oct 27, 2023 •
edited
Loading

Resolves #444

Right now tooltips on default mode don't show an arrow. This PR adds an arrow and solves an issue with the tooltip disappearing when trying to interact with the content (such as a button).

Screenshots

image
image

Before

Screen.Recording.2023-10-27.at.12.24.16.AM.mov

After

Screen.Recording.2023-10-26.at.11.52.31.PM.mov

benjaminknox
benjaminknox commented Oct 27, 2023
View reviewed changes
src/components/tooltip/tooltip.svelte
@@ -31,17 +31,21 @@
let arrow: HTMLElement
let trigger: HTMLElement

let arrowPlacement: string = undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used as a classname to style the arrow borders

benjaminknox
benjaminknox commented Oct 27, 2023
View reviewed changes
src/components/tooltip/tooltip.svelte Outdated
Comment on lines 63 to 80
const debouncedMouseleave = (event: MouseLeaveEvent) => {
let debounce

return () => {
clearTimeout(debounce)

debounce = setTimeout(() => {
if (!contentHovered) {
setVisible(false)
}
}, 300)
}
}
Copy link
Contributor Author

@benjaminknox benjaminknox Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gives the cursor some time to hover over the content in the tooltip, in case there is some action that can be done in the tooltip.

benjaminknox
benjaminknox commented Oct 27, 2023
View reviewed changes
src/components/floating/floating.svelte
@@ -96,7 +96,7 @@
}
</script>

<div bind:this={floating} class="leo-floating">
<div on:mouseenter on:mouseleave bind:this={floating} class="leo-floating">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to expose these so I can control when the tooltip should close

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like we should do this more!

@benjaminknox benjaminknox mentioned this pull request Oct 27, 2023
Closed
@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch from 14e5d50 to ed40285 Compare October 27, 2023 20:58
fallaciousreasoning
fallaciousreasoning previously approved these changes Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

src/components/floating/floating.svelte
@@ -96,7 +96,7 @@
}
</script>

<div bind:this={floating} class="leo-floating">
<div on:mouseenter on:mouseleave bind:this={floating} class="leo-floating">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like we should do this more!

@fallaciousreasoning
Copy link
Collaborator

fallaciousreasoning commented Oct 29, 2023 •
edited
Loading

Oh, looks like the npm run check step is failing (I think there's a MouseLeaveEvent which should be MouseEvent.

Also, noticed a little bug while testing. If you mouseover the trigger, to open a tooltip, mouse into the tooltip and then mouse into the trigger the tooltip will be closed.

Screencast from 2023-10-30 10-35-21.webm
(annoyingly you can't see my mouse here, but it goes Button -> tooltip -> button and stays hovered on the button)

@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch from 7229afb to 16788b6 Compare October 29, 2023 22:53
fallaciousreasoning
fallaciousreasoning previously approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending fix of the hover issue

@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch from f29fec9 to 8501a8e Compare October 30, 2023 01:25
@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch 4 times, most recently from 104724e to 9b481ff Compare October 30, 2023 03:41
@benjaminknox
Copy link
Contributor Author

Also, noticed a little bug while testing. If you mouseover the trigger, to open a tooltip, mouse into the tooltip and then mouse into the trigger the tooltip will be closed.

Latest should fix this issue:

Screen.Recording.2023-10-29.at.11.44.47.PM.mov
Screen.Recording.2023-10-29.at.11.44.05.PM.mov

@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch from 6604c7c to 5eb81c6 Compare October 30, 2023 17:37
@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch 2 times, most recently from 2873d63 to eda5c2c Compare October 30, 2023 21:45
@benjaminknox benjaminknox force-pushed the bpk/adds-arrow-on-tooltip-default-mode branch from eda5c2c to 1a1111a Compare October 30, 2023 21:49
fallaciousreasoning
fallaciousreasoning approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fallaciousreasoning fallaciousreasoning merged commit 87923ab into brave:main Oct 31, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fallaciousreasoning fallaciousreasoning fallaciousreasoning approved these changes

@petemill petemill Awaiting requested review from petemill petemill is a code owner

@AlanBreck AlanBreck Awaiting requested review from AlanBreck AlanBreck is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an arrow to tooltip
2 participants
@benjaminknox @fallaciousreasoning