cheton commented 1 month ago

Tooltip

https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-897/components/tooltip

By default, the Tooltip component has the placement prop set to bottom and the offset prop set to [0, 8]. In this setup, the first value in the offset array controls the skidding, while the second value determines the distance. You may need to modify the placement and offset props to suit your specific requirements better.

OverflowTooltip

https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-897/components/overflow-tooltip

By default, the OverflowTooltip component has the placement prop set to bottom-end and the offset prop set to [8, 12]. In this setup, the first value in the offset array controls the skidding, while the second value determines the distance. You may need to modify the placement and offset props to suit your specific requirements better.

PR Type

Enhancement, Documentation

Description

Updated tooltip examples to use placement prop instead of nextToCursor.
Added menus for selecting tooltip placement in various examples.
Adjusted default skidding and distance values for tooltips.
Refactored offset calculation logic in TooltipContent.
Documented default placement and offset props for OverflowTooltip and Tooltip components.

Changes walkthrough 📝

Relevant files

Enhancement

7 files

faq-relative-to-reference-element.js `Update tooltip placement in OverflowTooltip example.` packages/react-docs/pages/components/overflow-tooltip/faq-relative-to-reference-element.js Changed `nextToCursor` prop to `placement` prop with value `bottom`.	+1/-1
positioning-follow-cursor.js `Add placement selection and update tooltip positioning.` packages/react-docs/pages/components/tooltip/positioning-follow-cursor.js Added menu for selecting tooltip placement. Updated default skidding and distance values. Adjusted tooltip dimensions.	+72/-9
positioning-next-cursor.js `Add placement selection and update tooltip positioning.` packages/react-docs/pages/components/tooltip/positioning-next-cursor.js Added menu for selecting tooltip placement. Updated default skidding and distance values. Adjusted tooltip dimensions.	+72/-9
positioning-offset.js `Add placement selection and update tooltip positioning.` packages/react-docs/pages/components/tooltip/positioning-offset.js Added menu for selecting tooltip placement. Updated default skidding and distance values. Adjusted tooltip dimensions.	+84/-6
OverflowTooltip.js `Add offset and placement props to OverflowTooltip.` packages/react/src/tooltip/OverflowTooltip.js - Added `offset` and `placement` props with default values.	+4/-0
Tooltip.js `Simplify tooltip placement logic.` packages/react/src/tooltip/Tooltip.js - Simplified placement logic.	+1/-1
TooltipContent.js `Refactor offset calculation logic in TooltipContent.` packages/react/src/tooltip/TooltipContent.js - Refactored offset calculation logic based on placement.	+57/-33

Documentation

2 files

index.page.mdx `Document default placement and offset for OverflowTooltip.` packages/react-docs/pages/components/overflow-tooltip/index.page.mdx Added important notice about default `placement` and `offset` props. Updated prop table with new default values.	+13/-8
index.page.mdx `Document default placement and offset for Tooltip.` packages/react-docs/pages/components/tooltip/index.page.mdx Added important notice about default `placement` and `offset` props. Updated examples to reflect new default values.	+14/-3

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codesandbox[bot] commented 1 month ago

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders
Open Preview

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 9f77a504685929c5ed0798b60341ff2ae14f55c0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | --------------- | ----- | | @tonic-ui/react | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codiumai-pr-agent-free[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Complex Calculation
The offset calculation logic in `TooltipContent.js` has been significantly refactored to include complex conditions and calculations based on the tooltip's placement and cursor position. This could potentially introduce bugs or performance issues due to the complexity and dependency on multiple conditions and external measurements. It is recommended to add unit tests specifically targeting these calculations to ensure their correctness under various scenarios. Default Prop Change
The default props for `offset` and `placement` have been changed in `OverflowTooltip.js`. This alteration affects all instances of `OverflowTooltip` across the project. It's crucial to ensure that these changes do not adversely affect existing functionalities where `OverflowTooltip` is used.

cheton commented 1 month ago

/improve

codiumai-pr-agent-free[bot] commented 1 month ago

PR Code Suggestions ✨

Latest suggestions up to 9f77a50

Category	Suggestion	Score
Enhancement	Add `aria-haspopup="true"` to the `MenuButton` for better accessibility ___ To improve the accessibility and user experience, consider adding `aria-haspopup="true"` to the `MenuButton` component. This attribute informs assistive technologies that the button controls the display of a menu. [packages/react-docs/pages/components/tooltip/positioning-follow-cursor.js [44-46]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-5435045d19a1d554b8291cec6959b4c2779fc2755d93ce5e18cfc8b259c807fdR44-R46) ```diff ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding `aria-haspopup="true"` improves accessibility by informing assistive technologies about the menu, enhancing the user experience for people using screen readers.	9
Possible issue	Add conditional rendering to the `placement` prop based on `nextToCursor` ___ Consider adding a conditional rendering check for the `placement` prop to ensure it only applies when `nextToCursor` is false. This prevents potential conflicts between cursor positioning and explicit placement. [packages/react-docs/pages/components/overflow-tooltip/faq-relative-to-reference-element.js [8]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-27eee1499b3c720ee9523c0ee7efb7e6c6a64eaddecf00fb4ae4bf5eb8ffd813R8-R8) ```diff ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion helps prevent potential conflicts between cursor positioning and explicit placement, improving the robustness of the component.	8
Maintainability	Refactor offset calculation to a separate function for clarity ___ Refactor the offset calculation in the `popperModifiers` to a separate function to improve code readability and maintainability. [packages/react/src/tooltip/TooltipContent.js [71-127]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-1f36317cf4d70085ec792a74b9088f7722f10ffc97999b65c3524729941a9722R71-R127) ```diff +const calculateOffset = ({ placement, reference, popper }) => { + let computedSkidding = ensureFiniteNumber(skidding); + let computedDistance = ensureFiniteNumber(distance); + ... + return [computedSkidding, computedDistance]; +}; + const popperModifiers = useMemo(() => [ { name: 'offset', options: { - offset: ({ placement, reference, popper }) => { - let computedSkidding = ensureFiniteNumber(skidding); - let computedDistance = ensureFiniteNumber(distance); - ... - return [computedSkidding, computedDistance]; - }, + offset: calculateOffset, }, }, ], [skidding, distance, tooltipTriggerElement, followCursor, nextToCursor, mousePageX, mousePageY]); ``` Suggestion importance[1-10]: 7 Why: This refactoring improves code readability and maintainability, making it easier to understand and modify the offset calculation logic in the future.	7
Possible bug	Add a default value for `placement` in `MenuList` to handle undefined cases ___ It's recommended to handle potential undefined or null values for `placement` in the `MenuList` component to prevent runtime errors. [packages/react-docs/pages/components/tooltip/positioning-offset.js [51-53]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-0fab21ea9e67d83aed81b5e07a461b3c37b70d231c130706b840187684e81ad3R51-R53) ```diff ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Handling potential undefined or null values for `placement` prevents runtime errors, enhancing the stability of the component.	6

Previous suggestions

Suggestions up to commit 49ee28e

Category	Suggestion	Score
Maintainability	Extract complex logic into a separate function for clarity ___ The `popperModifiers` logic is complex and could benefit from being extracted into a separate function or utility. This would make `TooltipContent` cleaner and focus more on rendering logic rather than computational logic. [packages/react/src/tooltip/TooltipContent.js [71-127]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-1f36317cf4d70085ec792a74b9088f7722f10ffc97999b65c3524729941a9722R71-R127) ```diff +const calculateOffset = ({ placement, reference, popper }) => { + ... + return [computedSkidding, computedDistance]; +}; + const popperModifiers = useMemo(() => [ { name: 'offset', options: { - offset: ({ placement, reference, popper }) => { - ... - }, + offset: calculateOffset, }, }, ], [skidding, distance, tooltipTriggerElement, followCursor, nextToCursor, mousePageX, mousePageY]); ``` Suggestion importance[1-10]: 8 Why: Extracting the complex `popperModifiers` logic into a separate function enhances code readability and maintainability by separating computational logic from rendering logic.	8
Maintainability	Use descriptive function names for better code readability ___ It's recommended to use a more descriptive variable name than `changeBy` for the function returned from `useSelection`. A name like `updatePlacement` would make the code more readable and clearly indicate its purpose. [packages/react-docs/pages/components/tooltip/positioning-follow-cursor.js [31]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-5435045d19a1d554b8291cec6959b4c2779fc2755d93ce5e18cfc8b259c807fdR31-R31) ```diff -const [placement, changePlacementBy] = useSelection('bottom-end'); +const [placement, updatePlacement] = useSelection('bottom-end'); ``` Suggestion importance[1-10]: 7 Why: The suggestion to rename `changePlacementBy` to `updatePlacement` improves code readability and maintainability by making the function's purpose clearer.	7
Enhancement	Add `minWidth` and `minHeight` to ensure consistent rendering across browsers ___ To prevent potential layout issues in different browsers, consider setting explicit `minWidth` and `minHeight` styles on the `Flex` component inside the `Tooltip` to ensure consistent rendering. [packages/react-docs/pages/components/tooltip/positioning-offset.js [114-123]](https://github.com/trendmicro-frontend/tonic-ui/pull/897/files#diff-0fab21ea9e67d83aed81b5e07a461b3c37b70d231c130706b840187684e81ad3R114-R123) ```diff Suggestion importance[1-10]: 6 Why: Adding `minWidth` and `minHeight` can help prevent potential layout issues and ensure consistent rendering across different browsers, though it is a minor enhancement.	6

codesandbox-ci[bot] commented 1 month ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

trendmicro-frontend-bot commented 1 month ago

Tonic UI Demo

2024-07-19 14:14:36 +0000 The PR #897 (9f77a504685929c5ed0798b60341ff2ae14f55c0) has been deployed to https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-897/

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.84%. Comparing base (0736f0e) to head (9f77a50).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## v2 #897 +/- ## ========================================== + Coverage 74.80% 74.84% +0.04% ========================================== Files 378 378 Lines 6152 6166 +14 ========================================== + Hits 4602 4615 +13 - Misses 1550 1551 +1 ``` | [Flag](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | Coverage Δ | | |---|---|---| | [-base](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [-github](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [-hooks](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [-icons](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [-system](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [codemod](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [react](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [theme](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | | [utils](https://app.codecov.io/gh/trendmicro-frontend/tonic-ui/pull/897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend) | `74.84% <100.00%> (+0.04%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=trendmicro-frontend#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

trendmicro-frontend / tonic-ui

feat(react): improve tooltip placement and offset calculations #897