codesandbox[bot]
commented
1 month ago Review or Edit in CodeSandbox
Open the branch in Web Editor • VS Code • Insiders
Open Preview
Closed tinaClin closed 1 month ago
codesandbox[bot]
commented
1 month ago Open the branch in Web Editor • VS Code • Insiders
Open Preview
changeset-bot[bot]
commented
1 month ago Latest commit: ae5fc61f6560a85db6bb1616ad599656a02742f5
The changes in this PR will be included in the next version bump.
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 | ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
| ⚡ Key issues to review Default Prop Change The default value of `nextToCursor` has been changed from `true` to `false`. This is a significant behavior change that should be clearly documented and communicated to the users to avoid unexpected behavior in existing implementations. Calculation Logic The logic for calculating tooltip position when `followCursor` or `nextToCursor` is true has been modified. Ensure that these changes are thoroughly tested, especially the new calculations involving `elementPageX`, `elementPageY`, and `elementHeight`. Utility Function The new `pixelize` utility function has been introduced to handle pixel value conversions. It's crucial to ensure that this function handles all edge cases, as incorrect implementations could affect multiple components where this function is utilized. |
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 
codecov[bot]
commented
1 month ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 74.80%. Comparing base (
08cb4d5) to head (ae5fc61).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
cheton
commented
1 month ago /ask The default value of nextToCursor should remain true in the OverflowTooltip component.
codiumai-pr-agent-free[bot]
commented
1 month ago The default value of nextToCursor should remain true in the OverflowTooltip component.
The PR changes the default value of the nextToCursor prop from true to false in the OverflowTooltip component. This change is intended to fix a misalignment issue with popup menu items.
If the default value of nextToCursor should remain true, then the proposed change in the PR would not be appropriate. Here are the steps to address this:
Revert the Change in OverflowTooltip.js:
- nextToCursor={false}
+ nextToCursor={true}Revert the Documentation Update in index.page.mdx:
-| nextToCursor | boolean | false | If `true`, the tooltip will be positioned next to the cursor. |
+| nextToCursor | boolean | true | If `true`, the tooltip will be positioned next to the cursor. |Alternative Fix:
If the misalignment issue still needs to be addressed while keeping nextToCursor as true, consider investigating other potential causes or solutions for the misalignment. This might involve adjusting other properties or logic within the OverflowTooltip component.
Please let me know if you need further assistance or if there are any additional requirements.
cheton
commented
1 month ago /improve
cheton
commented
1 month ago /review
cheton
commented
1 month ago @tinaClin Based on the suggestion provided by the PR agent, it might be worth trying the proposed improvement:
Consider adding a conditional rendering or a prop to control the
nextToCursorbehavior dynamically based on theisOverflowstate or other relevant conditions. This will make the tooltip behavior more flexible and adaptable to different UI scenarios.
-nextToCursor={true}
+nextToCursor={isOverflow}~This PR introduces a workaround solution to mitigate the issue of overflow tooltip misalignment with popup menu items.~
I have created issue #895 to track this problem, and we plan to migrate from Popper v2 to Floating UI in a future release.
cheton
commented
1 month ago @tinaClin The misalignment issue has been addressed in this commit by simplifying the skidding and distance calculation.
let _skidding = ensureFiniteNumber(skidding);
let _distance = ensureFiniteNumber(distance);
if (isHTMLElement(tooltipTriggerElement) && (followCursor || nextToCursor)) {
const rect = tooltipTriggerElement.getBoundingClientRect();
const elementX = rect.x + globalThis.scrollX;
const elementY = rect.y + globalThis.scrollY;
const elementHeight = rect.height;
_skidding += ensureFiniteNumber(mousePageX - elementX);
_distance += ensureFiniteNumber(mousePageY - elementY - elementHeight);
}https://sentry.io/answers/how-do-i-get-the-position-x-y-of-an-html-element/)
How do you find the (x, y) coordinate of an HTML element on your page? You might want to position a popup next to an element, create a fancy animation, or overlay elements seamlessly.
There are two coordinate systems in HTML:
Let’s call these page coordinates and window coordinates. Both of these axes have their origin points (0,0) at the top left of the page or window, with values increasing towards the bottom right.
To get the position of an HTML element in JavaScript relative to the window, use getBoundingClientRect():
<!DOCTYPE html>
<html lang="">
<body>
<div id="myElement"
style="position: absolute; left: 50px; top: 100px;">
Hello World
</div>
<script>
globalThis.setInterval(() => {
const element = document.getElementById("myElement");
const rect = element.getBoundingClientRect();
console.log("X: " + rect.x + ", Y: " + rect.y);
}, 1000);
</script>
</body>
</html>In this code, we call getBoundingClientRect() on an element positioned manually at (50,100). This returns a rectangle object, whose properties are the x and y coordinates we need. The interval timer will write a new value to the console every second.
The DOMRect object provides the following properties for your element: left, top, right, bottom, x, y, width, and height.
Although you can set negative values in CSS, getBoundingClientRect() always returns positive width and height values. The left and x values are the same, as are the top and y values — as long as no CSS transform has been applied to the element (more on that later).
The height and width values include the element’s border and padding, but not its margin value. To calculate the full height of an element you need to access its CSS:
let style = getComputedStyle(element);
let marginTop = parseFloat(style.marginTop);
let marginBottom = parseFloat(style.marginBottom);Using getBoundingClientRect(), the position of the div will change whenever you scroll on the page. If you want to get the absolute position of the element on the page, not relative to the window, add the number of scrolled pixels to the element’s window location using scrollY:
console.log("X: " + (rect.x + globalThis.scrollX) + ", Y: " + (rect.y + globalThis.scrollY));The parentheses around the sums are added for JavaScript to treat them as numbers and not concatenate them as strings.
If you have a complex page layout, such as one with containers with their own scroll windows (div with overflow: scroll), or one using CSS transformations, you may need to use other element properties to calculate the x and y positions.
First, define what the position of an element means in your layout. Is it relative to the page, the window, or another element on the page? It’s likely you still need only the window position of the element to position another element next to it.
The following properties of an element may be useful to you (all are in pixels):
offsetParent — The closest positioned ancestor element that is not static (the default positioning type).offsetLeft and offsetTop — The distance from the outer border of the current element (including its margin) to the padding edge of the offsetParent.scrollHeight — The total height of a scrollable element, including the children not visible in the current view that you have to scroll down to see.scrollTop — The distance from the top of a scrollable element that the user has scrolled down to. If the scrollbar is still at the top of the element then scrollTop is 0.clientHeight — The height of a scrollable element visible on the page.The getBoundingClientRect().height property of an element is different from its clientHeight. The clientHeight property includes the content and its padding but not its border, margin, or horizontal scrollbar. The getBoundingClientRect().height property includes the height of the content, padding, scrollbar, and borders, but not the margin.
You can loop through every offsetParent of an element until you return to the HTML body. If you sum all the offsetTop values along the way, you’ll have the total distance of the element from the start of the page. This will equal rect.y + globalThis.scrollY from earlier.
A transform is a CSS setting like transform: translate(5px, 6px);. A transform changes the position of an element visually, but not its original layout position in the document flow.
If an element has been moved with a transform, then getBoundingClientRect() will return left and top values from before the translation, and x and y values from after the translation — where the element appears to be visually. So you probably want to use the x and y values.
If you need to access the exact values of a transform, you can use getComputedStyle():
const transform = window.getComputedStyle(element).getPropertyValue('transform');
if (transform !== 'none') {
const matrixValues = transform.match(/matrix.*\((.+)\)/)[1].split(', ');
const tx = parseFloat(matrixValues[4]);
const ty = parseFloat(matrixValues[5]);
console.log('Shifted x by ' + tx);
console.log('Shifted y by ' + ty);
}The transform object will be a string, like matrix(1, 0, 0, 1, 5, 6), so we need to extract its values using a regex match. The last two values are the x and y translation values. If you are using a 3D transformation with matrix3d, you need to adjust the regex.
A CSS transform also might involve scaling, rotation, and shearing, which will affect the visual appearance of the element in the window, but not its starting position.
cheton
commented
1 month ago /review
codiumai-pr-agent-free[bot]
commented
1 month ago cheton
commented
1 month ago /improve
cheton
commented
1 month ago /improve
codiumai-pr-agent-free[bot]
commented
1 month ago Latest suggestions up to debd8fa
| Category | Suggestion | Score |
| Maintainability |
Clarify the behavior of
___
**Instead of setting | 9 |
Improve the readability and testability of tooltip position calculations by extracting them into a separate function___ **Refactor the calculation of_skidding and _distance to a separate function to improve code readability and maintainability. This function can then be tested independently.** [packages/react/src/tooltip/TooltipContent.js [88-89]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-1f36317cf4d70085ec792a74b9088f7722f10ffc97999b65c3524729941a9722R88-R89) ```diff -_skidding += ensureFiniteNumber(mousePageX - elementPageX); -_distance += ensureFiniteNumber(mousePageY - elementPageY - elementHeight); +[_skidding, _distance] = calculateTooltipPosition(mousePageX, mousePageY, elementPageX, elementPageY, elementHeight); +function calculateTooltipPosition(mouseX, mouseY, elemX, elemY, elemHeight) { + const skidding = ensureFiniteNumber(mouseX - elemX); + const distance = ensureFiniteNumber(mouseY - elemY - elemHeight); + return [skidding, distance]; +} + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Refactoring the calculation into a separate function improves code readability and maintainability, and allows for independent testing of the position calculation logic. | 8 | |
| Enhancement |
Enhance tooltip alignment control by adding specific modifiers to
___
**Consider using a more specific prop than | 8 |
Improve clarity and conciseness of the tooltip offset description___ **Consider rephrasing the added text to make it more concise and clear. Instead ofspecifying the default values in a separate sentence, integrate them directly into the description of the nextToCursor and followCursor props.**
[packages/react-docs/pages/components/tooltip/index.page.mdx [100]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-bf0a24c7fad2beecf382885fdc72d27cd4dcd9b3042052de23de34843ef6e33fR100-R100)
```diff
-> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`.In the `nextToCursor` and `followCursor` examples below, the `distance` value is set to 16. You may need to adjust this value to better suit your specific requirements. +> The default tooltip's offset is `[0, 8]` (`skidding`, `distance`). For `nextToCursor` and `followCursor`, the `distance` is 16, adjustable to fit specific needs. ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion improves the readability and conciseness of the tooltip offset description, making it easier to understand. However, the original text is already clear, so the improvement is minor. | 7 | |
| Clarity |
✅ Clarify the tooltip's arrow visibility condition___Suggestion Impact:The commit rephrased the tooltip's arrow visibility condition to improve clarity, aligning with the suggestion's intention. code diff: ```diff -| arrow | boolean | true | If `true`, an arrow will be added to the tooltip. Note that the arrow will not be visible if either `followCursor` or `nextToCursor` is set to `true`. | +| arrow | boolean | true | If `true`, adds an arrow to the tooltip. Note: The arrow is not visible when `followCursor` or `nextToCursor` is enabled. | ```current phrasing might be confusing as it implies the arrow property's ineffectiveness is conditional only on these props.** [packages/react-docs/pages/components/tooltip/index.page.mdx [171]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-bf0a24c7fad2beecf382885fdc72d27cd4dcd9b3042052de23de34843ef6e33fR171-R171) ```diff -| arrow | boolean | true | If `true`, an arrow will be added to the tooltip. Note that the arrow will not be visible if either `followCursor` or `nextToCursor` is set to `true`. | +| arrow | boolean | true | Adds an arrow to the tooltip when `true`. However, the arrow is not visible when `followCursor` or `nextToCursor` are enabled. | ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: The suggestion clarifies the condition under which the arrow will not be visible, improving the documentation's clarity and reducing potential confusion for users. | 8 |
| Usability |
Improve tooltip visibility and interaction by adding an offset when
___
**To ensure that the tooltip does not overlap with the cursor when | 7 |
| Category | Suggestion | Score |
| Maintainability |
✅ Remove duplicated content to enhance documentation clarity___Suggestion Impact:The commit removed the duplicated content about the tooltip's default offset settings, which aligns with the suggestion to enhance documentation clarity. code diff: ```diff +> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`.In the `nextToCursor` and `followCursor` examples below, the `distance` value is set to 16. You may need to adjust this value to better suit your specific requirements. + #### Using the `nextToCursor` prop The `nextToCursor` prop positions the tooltip next to the cursor. - -> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`. In this example, the `distance` value is set to 16. You may need to adjust this value to better suit your specific requirements. {render('./positioning-next-cursor')} #### Using the `followCursor` prop The `followCursor` prop makes the tooltip follow the cursor as it moves. - -> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`. In this example, the `distance` value is set to 16. You may need to adjust this value to better suit your specific requirements. ``` maintain clarity and avoid redundancy in the documentation.** [packages/react-docs/pages/components/tooltip/index.page.mdx [104-112]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-bf0a24c7fad2beecf382885fdc72d27cd4dcd9b3042052de23de34843ef6e33fR104-R112) ```diff -> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`. In this example, the `distance` value is set to 16. You may need to adjust this value to better suit your specific requirements. +> By default, the tooltip's offset is set to `[0, 8]`, where the first value represents the `skidding` and the second value represents the `distance`. ``` Suggestion importance[1-10]: 9Why: The suggestion correctly identifies and removes redundant content, improving the clarity and maintainability of the documentation. However, it slightly reduces the context provided to the reader. | 9 |
✅ Simplify the conditional assignment in
___
| 6 | |
| Enhancement |
✅ Standardize and clarify the description for the
___
**Update the description for the | 8 |
Replace React.Fragment with fragment shorthand for cleaner code___ **Consider using a fragment shorthand<> instead of your React component.** [packages/react-docs/pages/components/overflow-tooltip/faq-misalignment-with-menu-items.js [26-83]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-505ab527b6874ab57cdb1bbd2b740f5ad41452b78bca7eaf0fef40e5fab7e193R26-R83) ```diff return ( - + <> + + ); ``` Suggestion importance[1-10]: 3Why: The suggestion to use fragment shorthand improves code readability slightly, but it does not address any functional or significant stylistic issues. The existing code is already clear and correct. | 3 | |
| Accessibility |
Add accessible labels to range inputs___ **To improve the accessibility and usability of the range input forskidding, consider adding labels that are visually hidden but accessible to screen readers.** [packages/react-docs/pages/components/tooltip/positioning-follow-cursor.js [16-22]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-5435045d19a1d554b8291cec6959b4c2779fc2755d93ce5e18cfc8b259c807fdR16-R22) ```diff - setSkidding(Number(e.target.value))} -/> +<> + + setSkidding(Number(e.target.value))} + /> + ``` Suggestion importance[1-10]: 8Why: Adding accessible labels to range inputs significantly improves accessibility for screen reader users, making the application more inclusive. | 8 |
| Best practice |
Add error handling for the
___
**It's a good practice to handle potential errors or exceptions when using external | 7 |
| Category | Suggestion | Score |
| Enhancement |
Make
___
**To ensure that the | 9 |
| Maintainability |
Extract repeated long strings into a constant for better maintainability___ **Consider extracting the repeated long string into a constant to avoid redundancy andimprove maintainability. This will make the code cleaner and easier to update in the future if the string needs to be changed.** [packages/react-docs/pages/components/overflow-tooltip/with-menu-item.js [31-52]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-dd2e458c986224d362630414e0ca14cd4d85148d9bd6e70f7c87efc770c3d8faR31-R52) ```diff -This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string. +const LONG_STRING = "This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string.This is a very very long string."; + Suggestion importance[1-10]: 8Why: Extracting the repeated long string into a constant improves code maintainability and readability, making future updates easier. | 8 |
| Performance |
Use React.memo for
___
**To improve performance and avoid potential memory leaks, consider using React.memo | 8 |
| Best practice |
Use CSS classes instead of inline styles for better maintainability___ **Replace inline styles with a styled component or use a CSS class for theMenu and Scrollbar components to improve code readability and maintainability. Inline styles can be harder to manage and override in larger applications.** [packages/react-docs/pages/components/overflow-tooltip/with-menu-item.js [25-28]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-dd2e458c986224d362630414e0ca14cd4d85148d9bd6e70f7c87efc770c3d8faR25-R28) ```diff - | 7 |
| Category | Suggestion | Score |
| Documentation |
✅ Update documentation to match dynamic behavior of the tooltip___Suggestion Impact:The commit updated the documentation to reflect the dynamic behavior of the `nextToCursor` prop, mentioning its default behavior and how to change it. code diff: ```diff +### Misalignment with menu items + +The `OverflowTooltip` is positioned next to the cursor by default. However, when used within a popup menu, the tooltip may not align correctly with the menu items. + +To resolve this issue, set `PopperProps={{ usePortal: true }}`. + +{render('./faq-misalignment-with-menu-items')} + +### Aligning the `OverflowTooltip` relative to the reference element + +By default, the `OverflowTooltip` component is positioned next to the cursor with the `nextToCursor` prop set to `true`. To align the tooltip relative to the reference element, set the `nextToCursor` prop to `false`. ```nextToCursor prop if it's made dynamic in the component. This ensures that the documentation stays in sync with the component's actual behavior.** [packages/react-docs/pages/components/overflow-tooltip/index.page.mdx [58]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-1aae44fa4234a8035fb3160196d204005d35ee731b56ba07809cfe89c2254ecaR58-R58) ```diff -| nextToCursor | boolean | false | If `true`, the tooltip will be positioned next to the cursor. | +| nextToCursor | boolean | depends on overflow | Tooltip positioning next to the cursor depends on the overflow state. | ``` Suggestion importance[1-10]: 8Why: Updating the documentation to reflect the dynamic behavior of the `nextToCursor` prop is essential for maintaining accurate and helpful documentation. This ensures that users are aware of how the prop behaves in different scenarios. | 8 |
| Enhancement |
✅ Make the tooltip positioning dynamic based on overflow state___Suggestion Impact:The commit introduced a dynamic behavior for the nextToCursor prop based on the isOverflow state, aligning with the suggestion's intention to improve flexibility and adaptability of the tooltip behavior. code diff: ```diff + arrow = false, children, + nextToCursor = true, ...rest }, ref, @@ -57,12 +59,16 @@ useEventListener(eventTargetFn, 'mouseenter', onMouseEnter); useEventListener(eventTargetFn, 'mouseleave', onMouseLeave); + const tooltipProps = { + arrow, + disabled: !isOverflow, + ...(isOverflow && { nextToCursor }) + }; + return (nextToCursor behavior dynamically based on the isOverflow state or other relevant conditions. This will make the tooltip behavior more flexible and adaptable to different UI scenarios.** [packages/react/src/tooltip/OverflowTooltip.js [65]](https://github.com/trendmicro-frontend/tonic-ui/pull/894/files#diff-700800625e63a517f10bbd9526e1c55b686c832fdc7fc7e2adb61ba4a23c644aR65-R65) ```diff -nextToCursor={false} +nextToCursor={!isOverflow} ``` Suggestion importance[1-10]: 7Why: The suggestion to make the `nextToCursor` prop dynamic based on the `isOverflow` state can improve the flexibility and adaptability of the tooltip behavior. However, the proposed code `nextToCursor={!isOverflow}` is incorrect because it inverts the intended logic. The correct implementation should be `nextToCursor={isOverflow}`. | 7 |
User description
https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-894/components/overflow-tooltip
The default value of
nextToCursoristrue, in accordance with the design specifications. Therefore, this PR merely updates the documentation to detail how to address this UI issue.Issue: https://github.com/trendmicro-frontend/tonic-ui/issues/893
PR Type
Documentation
Description
OverflowTooltipcomponent by settingnextToCursorprop tofalse.nextToCursorprop.Changes walkthrough 📝
OverflowTooltip.js
Fix tooltip misalignment by updating `nextToCursor` proppackages/react/src/tooltip/OverflowTooltip.js
nextToCursorprop fromtruetofalseinOverflowTooltipcomponent.
index.page.mdx
Update documentation for `nextToCursor` prop default valuepackages/react-docs/pages/components/overflow-tooltip/index.page.mdx
nextToCursorprop.