fix: increase z-index for CopyToClipboardButton#2701
fix: increase z-index for CopyToClipboardButton#2701gameroman wants to merge 6 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIncreases CopyToClipboardButton tooltip z-index from ChangesClipboard Button Z-Index & E2E Validation
Suggested reviewers:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/interactions.spec.ts (1)
119-129: ⚡ Quick winConsider making the occlusion check more specific.
The current implementation verifies that the element at each point is within any
button[aria-label], which could lead to false positives if another button with anaria-labelattribute is added near the copy button in future UI changes.For a more robust test, verify that the element at each point belongs specifically to the copy button by comparing its
aria-labelvalue.♻️ Suggested refinement
+ // Get the specific aria-label to verify we're checking the right button + const ariaLabel = await copyButton.getAttribute('aria-label') + if (!ariaLabel) throw new Error('Copy button has no aria-label') + for (const { x, y } of points) { const isOnTop = await page.evaluate( - ({ x, y }) => { + ({ x, y, expectedLabel }) => { const el = document.elementFromPoint(x, y) - // Ensure the element at this point is the button or contained within it - return el?.closest('button[aria-label]') !== null + const button = el?.closest('button[aria-label]') + return button?.getAttribute('aria-label') === expectedLabel }, - { x, y }, + { x, y, expectedLabel: ariaLabel }, ) expect(isOnTop, `Button is occluded at point (${x.toFixed(0)}, ${y.toFixed(0)})`).toBe(true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/interactions.spec.ts` around lines 119 - 129, The occlusion check is too broad because it accepts any button with an aria-label; update the loop that uses page.evaluate (iterating over points) so the callback checks the element at document.elementFromPoint(x,y) has a closest('button[aria-label]') whose getAttribute('aria-label') strictly equals the copy button's expected aria-label (e.g., the known label string for the copy button) instead of just testing for non-null; keep using the same points array and expect message but assert that the aria-label matches the specific copy button's label.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/interactions.spec.ts`:
- Around line 119-129: The occlusion check is too broad because it accepts any
button with an aria-label; update the loop that uses page.evaluate (iterating
over points) so the callback checks the element at
document.elementFromPoint(x,y) has a closest('button[aria-label]') whose
getAttribute('aria-label') strictly equals the copy button's expected aria-label
(e.g., the known label string for the copy button) instead of just testing for
non-null; keep using the same points array and expect message but assert that
the aria-label matches the specific copy button's label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f97f8a16-985c-4393-98c4-8e68e24c042b
📒 Files selected for processing (1)
test/e2e/interactions.spec.ts
| await goto('/package/vue', { waitUntil: 'hydration' }) | ||
|
|
||
| const packageHeading = page.locator('h1').first() | ||
| await expect(packageHeading).toBeVisible({ timeout: 15000 }) |
There was a problem hiding this comment.
is a 15s timeout really necessary ?
There was a problem hiding this comment.
Probably not, let me check
Does it matter in case only if it fails or how does it work?
There was a problem hiding this comment.
Although, this matches timeouts for other tests I think, so I am not sure
gameroman
force-pushed
the
fix-increase-z-index-CopyToClipboardButton
branch
from
e748ae0 to
3c23514
Compare
|
|
||
| for (const { x, y } of points) { | ||
| const isOnTop = await page.evaluate( | ||
| ({ x, y }) => { |
There was a problem hiding this comment.
Can you address this eslint warning ?
For example:
const isOnTop = await page.evaluate(
({ pointX, pointY }) => {
const element = document.elementFromPoint(pointX, pointY)
return element?.closest('button[aria-label]') !== null
},
{ pointX: x, pointY: y },
)2 participants
🔗 Linked issue
Closes #2700
🧭 Context
📚 Description
Fixed
CopyToClipboardButton'sz-index