Setup Playwright tests, misc fixes #4
Conversation
✅ Deploy Preview for molevolvr ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I've just added a commit that pulls in updates from krishnanlab/geneplexus-app-v2#38, which includes unrelated fixes, but also some accessibility fixes that would affect the Axe testing here. It also adds |
vincerubinetti
requested review from
falquaddoomi
and removed request for
falquaddoomi
|
Hey, do you expect people to run these tests you've added locally, or are they only for the GitHub action? If I can run them locally, how do I do that? I'm getting the following when I try to do what seems obvious from reading the scripts you've defined in |
|
Yes you should be able to run them locally. Do you have node v22 installed? |
|
Aha; I wasn't using node v22 at the time, but I switched to Perhaps you should specify the version of node that's required via https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines, or some other way like in the README? Also, adding something to your PR description that specifies what you'd like me to test and how to do it would be nice. |
|
Sorry, I thought that was already in the readme. I've updated the readme to add some clarifications.
What I can do from now on is make a comment near sections of the diff that are particularly complex/error-prone/high-consequence. I did this for monarch and it helped them ignore irrelevant changes. I will do this now for this PR. |
| * is element covering anything "important" (above anything besides a | ||
| * "background" element) | ||
| */ | ||
| export const isCovering = ( |
There was a problem hiding this comment.
Util func that checks a grid of points under an element.
There was a problem hiding this comment.
Interesting solution, and it seems to work well.
I'm not suggesting changing it, but if you're interested you might consider trying the intersection observer API, which seems to have good support from all the major browsers. Basically it allows you to register a target element that gets checked against all the elements under some node in the DOM (by default it's the browser window, i.e. all elements) for intersections and fires a callback when an intersection occurs. Supposedly it's more performant than checking yourself, but since you're just sampling a few points I think your solution is fine.
There was a problem hiding this comment.
I'm not sure if ResizeObserver can be used in this manner, because although you can specify a root element other than the viewport, it seems that the root must be an ancestor. That is, the element being covered (let's say an svg chart element) would have to be a child of the table of contents.
Idk if there is a browser API I could use to make this less kludgy. I could do a simple bbox overlap check, but then I'd have to do it against all the elements in the viewport. It might end up being faster than this method though.
There was a problem hiding this comment.
Ah, I thought it could check intersections between the target and any child of the root element, but I guess if it's just the root vs. the target then it's not very useful.
I'm curious if iterating over all the visible elements and doing bbox checks would be faster than elementsFromPoint() (no idea how that function works under the hood), but IMHO what you have is good. I'd only explore it if you're curious and have time, or if you run into performance issues.
There was a problem hiding this comment.
I switched to the bbox method. I found the elementsFromPoint method to range from < 1ms to 10ms, and the bbox method to always be < 1ms. Since this function is run in a very debounced fashion, this should never really matter, but it could matter if we needed to call it on every scroll event in the future. The bbox method, with the default "important" element selector, should definitely do less work on average than elementsFromPoint.
Bbox method, at least with the default section > * selector, will be less "accurate", in the sense that it's based on DOM hierarchy and not actual page layout, which are not always the same. For example:
<section>
<!-- narrow box in center of screen. gets checked by util func by default, b/c is a direct descendant of section -->
<div style="width: 100px">
<!-- image that is "popped out" of regular document flow, positioned outside its parent div, floating on the left side of the screen. not checked by the util func by default, but could still be under the floating table of contents panel -->
<img style="position: fixed; left: 0"/>
</div>
</section>This is unlikely HTML/CSS layout though. If something needs to be popped out like this, its likely to be done at the top level (the div in the example).
If it becomes a problem, could change the selector to section * (would have to check many more elements, becoming possibly slower than the elementsFromPoint method).
Thanks; I realize this took effort, and it is helpful. What I meant, though, was just some simple directions for things you'd want tried out by a reviewer, e.g. "Run You're of course welcome to continue to describe the significant code changes in your PR and why you made them, but I don't expect you to. |
|
Gotcha. I'll still try to do the comment-by-comment when possible, still seems helpful on large PRs so you can sort of skip over lower risk/impact things. And if not I'll have a paragraph or so summarizing the salient things. |
|
Just FYI, I've looked through the changes and I still approve it all. Feel free to merge. |
Note that the diff here is a pain because I had to do a bulk
chmoddue to a playwright setup command messing with a bunch of file permissions, and couldn't find a way to reset the permissions to their defaults (pre-playwright-command) without going through one file at a time.