-
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
chore: Attaches tooltip classes to popover container #3219
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3219 +/- ##
==========================================
+ Coverage 96.42% 96.44% +0.01%
==========================================
Files 787 790 +3
Lines 22191 22246 +55
Branches 7555 7229 -326
==========================================
+ Hits 21398 21455 +57
- Misses 741 784 +43
+ Partials 52 7 -45 ☔ View full report in Codecov by Sentry. |
@@ -39,7 +38,7 @@ export default function Tooltip({ | |||
|
|||
return ( | |||
<Portal> | |||
<div className={clsx(styles.root, className)} {...contentAttributes} data-testid={trackKey}> | |||
<div className={styles.root} {...contentAttributes} data-testid={trackKey}> |
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.
Why is styles.root
staying here? it is also a test-util only style, what is the use-case of selecting it, if it is incorrectly placed in the DOM?
Why does this DOM node needed in the first place?
There is an avatar test util pointing on this DOM node too: https://github.com/cloudscape-design/chat-components/blob/81f675f568a82891661839f382262b80ea276289/src/test-utils/dom/avatar/index.ts#L14
Do you plan updating it in a separate PR?
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.
That's a good call out. I will create follow-up PRs that will 1) Remove the dependency on the tooltip wrapper from Avatar and 2) Remove the root style from this node.
I will also check if there is an opportunity to remove the node altogether. I think the "contentAttributes" and the testid can be passed down to the internal container.
Description
The tooltip test classes were attached to the div that contains a popover container. However, when the container becomes hidden due to scroll logic, the outer div stays and gives a false positive when checked with isDisplayed if using webdriverio v9. In the PR the classes are moved to the popover container so that the tests produce expected results.
How has this been tested?
See a failed dry-run: https://github.com/cloudscape-design/browser-test-tools/actions/runs/12945599198/job/36179765443?pr=109
The tests succeed with the fix applied: #3218
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.