-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Add mouseover
and mouseout
as default events
#1194
Conversation
WalkthroughThe changes update the Tooltip component in a React application to enhance event handling and refine prop management. Deprecated props are removed, default event values are adjusted, and new global close events are added. These modifications improve tooltip behavior, especially for nested elements, ensuring tooltips update correctly when moving between parent and child elements. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Beta version released with the last commit 🚀
or
|
@@ -88,13 +88,15 @@ export interface TooltipRefProps { | |||
export type AnchorOpenEvents = { | |||
mouseenter?: boolean |
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.
Should we deprecate mouseenter
and mouseleave
?
Keeping them seems fine, but with mouseover
/mouseout
, would they ever really be useful?
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.
What will be the benefits to keep and to remove?
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.
I honestly can't think of any realistic situation mouseenter
/mouseleave
(1) would solve something that mouseover
/mouseout
(2) doesn't.
The only issue with (2) is that we're triggering sequential hide
/show
state updates on the tooltip every time the user hovers between the nested element and the parent element. I couldn't visually notice any bad behavior due to this, but it's something to keep in mind.
Maybe we just leave (1) as the default, and have (2) + a troubleshooting page for people having issues with nested anchor elements?
Furthermore, maybe a more robust approach would be to forget about (2) and handle nested elements explicitly (use DOM element contains()
method)?
There's a few different approaches we can take here. We can come back to this discussion later.
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.
Got it, about removing it: if it's not causing any bug, I would like to keep it so we support those events.
About the contains()
, I like the idea
Beta version released with the last commit 🚀
or
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docs/docs/options.mdx (1 hunks)
- src/components/Tooltip/Tooltip.tsx (4 hunks)
- src/components/Tooltip/TooltipTypes.d.ts (1 hunks)
Additional context used
LanguageTool
docs/docs/options.mdx
[uncategorized] ~95-~95: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ... | Class name to customize tooltip arrow element. You can also use the def...
[uncategorized] ~107-~107: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... ||hover
...hover
click
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnEsc
...boolean
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnEsc
|boolean
no| ~...
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnEsc
|boolean
no|...false
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... || ~~Pressing escape key will close the t...true
false
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnScroll
...boolean
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnScroll
|boolean
no| ~...
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnScroll
|boolean
no|...false
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... ||true
false
Scrolling will close the tooltip<...
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnResize
...boolean
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnResize
|boolean
no| ~...
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnResize
|boolean
no|...false
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... || ~~Resizing the window will close the t...true
false
[typographical] ~126-~126: Consider adding a comma. (IF_THEN_COMMA)
Context: ...own by default, if false or not provided then it's in hidden state by default. | | ...
Biome
src/components/Tooltip/Tooltip.tsx
[error] 867-867: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 868-868: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 874-874: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 875-875: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 899-899: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 900-900: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 902-902: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 222-222: This hook does not specify all of its dependencies: afterHide (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 222-222: This hook does not specify all of its dependencies: afterShow (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 412-412: This hook does not specify all of its dependencies: border (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 412-412: This hook does not specify all of its dependencies: handleTooltipPosition (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
[error] 412-412: This hook specifies more dependencies than necessary: show, content, externalStyles (lint/correctness/useExhaustiveDependencies)
This dependency can be removed from the list.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
[error] 412-412: This hook does not specify all of its dependencies: handleComputedPosition (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 412-412: This hook does not specify all of its dependencies: middlewares (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 469-469: This hook does not specify all of its dependencies: actualCloseEvents (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 469-469: This hook does not specify all of its dependencies: actualGlobalCloseEvents.escape (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
[error] 469-469: This hook does not specify all of its dependencies: anchorId (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
[error] 469-469: This hook does not specify all of its dependencies: handleShow (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
[error] 469-469: This hook specifies more dependencies than necessary: rendered, openEvents, closeEvents, globalCloseEvents, shouldOpenOnClick, delayShow, delayHide (lint/correctness/useExhaustiveDependencies)
This dependency can be removed from the list.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
This dependency can be removed from the list.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
Outer scope values aren't valid dependencies because mutating them doesn't re-render the component.
[error] 469-469: This hook does not specify all of its dependencies: show (lint/correctness/useExhaustiveDependencies)
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
Additional comments not posted (4)
src/components/Tooltip/TooltipTypes.d.ts (2)
91-91
: LGTM! The addition ofmouseover
toAnchorOpenEvents
aligns with the PR's objectives.
99-99
: LGTM! The addition ofmouseout
toAnchorCloseEvents
aligns with the PR's objectives.docs/docs/options.mdx (1)
91-135
: LGTM! The documentation updates accurately reflect the changes in the Tooltip component's behavior and properties.Tools
LanguageTool
[uncategorized] ~95-~95: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ... | Class name to customize tooltip arrow element. You can also use the def...
[uncategorized] ~107-~107: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... ||hover
...hover
click
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnEsc
...boolean
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnEsc
|boolean
no| ~...
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnEsc
|boolean
no|...false
[uncategorized] ~116-~116: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... || ~~Pressing escape key will close the t...true
false
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnScroll
...boolean
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnScroll
|boolean
no| ~...
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnScroll
|boolean
no|...false
[uncategorized] ~117-~117: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... ||true
false
Scrolling will close the tooltip<...
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnResize
...boolean
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... | ||closeOnResize
|boolean
no| ~...
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ...|closeOnResize
|boolean
no|...false
[uncategorized] ~118-~118: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... || ~~Resizing the window will close the t...true
false
[typographical] ~126-~126: Consider adding a comma. (IF_THEN_COMMA)
Context: ...own by default, if false or not provided then it's in hidden state by default. | | ...src/components/Tooltip/Tooltip.tsx (1)
Line range hint
105-134
: LGTM! The updates to the Tooltip component's event handling logic align with the PR's objectives and are well-implemented.
## [0.6.5](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@0.6.5) (2024-06-24) ### Bug Fixes * bump react-tooltip from 4.5.1 to 5.27.0 in /typescript ([#2104](#2104)) ([28bd015](28bd015)), closes [ReactTooltip/react-tooltip#1148](ReactTooltip/react-tooltip#1148) [ReactTooltip/react-tooltip#1196](ReactTooltip/react-tooltip#1196) [ReactTooltip/react-tooltip#1194](ReactTooltip/react-tooltip#1194) [ReactTooltip/react-tooltip#1187](ReactTooltip/react-tooltip#1187) [ReactTooltip/react-tooltip#1171](ReactTooltip/react-tooltip#1171) [ReactTooltip/react-tooltip#1169](ReactTooltip/react-tooltip#1169) [ReactTooltip/react-tooltip#1168](ReactTooltip/react-tooltip#1168) [ReactTooltip/react-tooltip#1170](ReactTooltip/react-tooltip#1170) [ReactTooltip/react-tooltip#1172](ReactTooltip/react-tooltip#1172) [#1196](#1196) [#1148](#1148)
## [1.2.17](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.2.17) (2024-06-24) ### Bug Fixes * bump react-tooltip from 4.5.1 to 5.27.0 in /typescript ([#2104](#2104)) ([28bd015](28bd015)), closes [ReactTooltip/react-tooltip#1148](ReactTooltip/react-tooltip#1148) [ReactTooltip/react-tooltip#1196](ReactTooltip/react-tooltip#1196) [ReactTooltip/react-tooltip#1194](ReactTooltip/react-tooltip#1194) [ReactTooltip/react-tooltip#1187](ReactTooltip/react-tooltip#1187) [ReactTooltip/react-tooltip#1171](ReactTooltip/react-tooltip#1171) [ReactTooltip/react-tooltip#1169](ReactTooltip/react-tooltip#1169) [ReactTooltip/react-tooltip#1168](ReactTooltip/react-tooltip#1168) [ReactTooltip/react-tooltip#1170](ReactTooltip/react-tooltip#1170) [ReactTooltip/react-tooltip#1172](ReactTooltip/react-tooltip#1172) [#1196](#1196) [#1148](#1148)
## [0.27.9](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@0.27.9) (2024-06-24) ### Bug Fixes * bump react-tooltip from 4.5.1 to 5.27.0 in /typescript ([#2104](#2104)) ([28bd015](28bd015)), closes [ReactTooltip/react-tooltip#1148](ReactTooltip/react-tooltip#1148) [ReactTooltip/react-tooltip#1196](ReactTooltip/react-tooltip#1196) [ReactTooltip/react-tooltip#1194](ReactTooltip/react-tooltip#1194) [ReactTooltip/react-tooltip#1187](ReactTooltip/react-tooltip#1187) [ReactTooltip/react-tooltip#1171](ReactTooltip/react-tooltip#1171) [ReactTooltip/react-tooltip#1169](ReactTooltip/react-tooltip#1169) [ReactTooltip/react-tooltip#1168](ReactTooltip/react-tooltip#1168) [ReactTooltip/react-tooltip#1170](ReactTooltip/react-tooltip#1170) [ReactTooltip/react-tooltip#1172](ReactTooltip/react-tooltip#1172) [#1196](#1196) [#1148](#1148)
## [1.1.17](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.1.17) (2024-06-24) ### Bug Fixes * bump react-tooltip from 4.5.1 to 5.27.0 in /typescript ([#2104](#2104)) ([28bd015](28bd015)), closes [ReactTooltip/react-tooltip#1148](ReactTooltip/react-tooltip#1148) [ReactTooltip/react-tooltip#1196](ReactTooltip/react-tooltip#1196) [ReactTooltip/react-tooltip#1194](ReactTooltip/react-tooltip#1194) [ReactTooltip/react-tooltip#1187](ReactTooltip/react-tooltip#1187) [ReactTooltip/react-tooltip#1171](ReactTooltip/react-tooltip#1171) [ReactTooltip/react-tooltip#1169](ReactTooltip/react-tooltip#1169) [ReactTooltip/react-tooltip#1168](ReactTooltip/react-tooltip#1168) [ReactTooltip/react-tooltip#1170](ReactTooltip/react-tooltip#1170) [ReactTooltip/react-tooltip#1172](ReactTooltip/react-tooltip#1172) [#1196](#1196) [#1148](#1148)
## [1.10.1](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.10.1) (2024-06-24) ### Bug Fixes * bump react-tooltip from 4.5.1 to 5.27.0 in /typescript ([#2104](#2104)) ([28bd015](28bd015)), closes [ReactTooltip/react-tooltip#1148](ReactTooltip/react-tooltip#1148) [ReactTooltip/react-tooltip#1196](ReactTooltip/react-tooltip#1196) [ReactTooltip/react-tooltip#1194](ReactTooltip/react-tooltip#1194) [ReactTooltip/react-tooltip#1187](ReactTooltip/react-tooltip#1187) [ReactTooltip/react-tooltip#1171](ReactTooltip/react-tooltip#1171) [ReactTooltip/react-tooltip#1169](ReactTooltip/react-tooltip#1169) [ReactTooltip/react-tooltip#1168](ReactTooltip/react-tooltip#1168) [ReactTooltip/react-tooltip#1170](ReactTooltip/react-tooltip#1170) [ReactTooltip/react-tooltip#1172](ReactTooltip/react-tooltip#1172) [#1196](#1196) [#1148](#1148)
Closes #1193
See this demo for explanation.
mouseover
andmouseout
apply to a wider range of scenarios, more specifically when dealing with nested tooltip anchors.mouseenter
andmouseleave
should still be present for backwards compatibility. But should we deprecate it and remove it on v6? Keeping it seems fine for more functionality, but will likely almost never be used.This will not impact current behavior for "regular" tooltip usage (no nested anchor elements).
Summary by CodeRabbit
New Features
mouseover
andmouseout
.Bug Fixes
Documentation