#8380: add isInViewport property to element reader #8381
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8381 +/- ##
==========================================
+ Coverage 73.47% 73.48% +0.01%
==========================================
Files 1334 1334
Lines 41251 41259 +8
Branches 7679 7684 +5
==========================================
+ Hits 30308 30319 +11
+ Misses 10943 10940 -3 ☔ View full report in Codecov by Sentry. |
Playwright test results - MV2
Details
Flaky testsedgeSetup › auth.setup.ts › authenticate Skipped testschrome › tests/extensionConsoleActivation.spec.ts › can activate a mod with built-in integration |
Playwright test results - MV3
Details
|
| /** | ||
| * Returns true if the element is completely in the viewport. | ||
| */ | ||
| export function isInViewport(element: HTMLElement): boolean { |
There was a problem hiding this comment.
We should note on this method that this doesn't work for elements hidden by z-index overlapped, or if they are hidden by overflow-scroll in element's container. This may also have issues with things hidden by relative/absolute positioning, and if they are within an iframe that is not entirely visible in the top frame.
| export function isInViewport(element: HTMLElement): boolean { | ||
| // https://stackoverflow.com/a/125106 | ||
| const rect = element.getBoundingClientRect(); | ||
| return ( |
There was a problem hiding this comment.
Should we also check isVisible?
There was a problem hiding this comment.
We also have isVisible exposed on ElementReader. So I think it's good to keep them separate.
Although, related to your comment here: #8381 (comment), the devil's advocate is that ideally for the customer's use case we'd expose a property that captures whether or not the element would be fully captured by our Screenshot Brick.
We just happen to know there's no funny business going on the site they need to screenshot, so the method as-is would be sufficient.
Perhaps there's a better name we could use the method/output property? Although, from the Google/SO Results, this is what most developers seem to call it. (Although a good proportion of them seem to use it to mean the element is at least partially in the viewport)
Some other name options for brainstorming:
- isFullyWithinViewport
- isPositionedWithinViewport
- isFullyPositionedWithinViewport
Although "position" might be confusing w.r.t. CSS position properties
There was a problem hiding this comment.
We just happen to know there's no funny business going on the site they need to screenshot
Maybe I'm missing the context, but is this for a specific customer?
I think it's fine to leave the methodName and functionality as-is, but I would recommend updating the jsdoc to be more comprehensive on the behavior and limitations of this function.
There was a problem hiding this comment.
Maybe I'm missing the context, but is this for a specific customer?
Yes. To see that you can look for the "customer" label and user story in the issue: #8380. Adding the customer name epic now
I think it's fine to leave the methodName and functionality as-is, but I would recommend updating the jsdoc to be more comprehensive on the behavior and limitations of this function.
👍
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
…brix-extension into feature/8380-in-viewport
twschiller
changed the title
What does this PR do?
Checklist