-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Add popup position and anchor #6414
Conversation
441d3db
to
f4cabe6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6414 +/- ##
==========================================
+ Coverage 88.50% 88.51% +0.01%
==========================================
Files 323 323
Lines 68620 68599 -21
==========================================
- Hits 60730 60722 -8
+ Misses 7890 7877 -13 ☔ View full report in Codecov by Sentry. |
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.
It's unfortunate that there is so much "almost" repeatable code, but I can't think of a better way to do it.
Because most of the logic is inside CustomJS, I'm leaning toward testing every position/geometry type case. Hopefully, this can be done with parameterized unit tests.
Also the test_stream_popup_polygons_selection1d
fails which is somewhat
Commit suggestions Co-authored-by: Simon Høxbro Hansen <[email protected]>
Okay I think I addressed your review comments; let me know if there's anything else. |
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.
We could benefit from having a class with some helper functions for the popup tests. There seems to be a lot of repeating code, which makes it somewhat hard to read.
The helper functions should have long descriptive names so we don't need to look into the functions to understand what happens and don't need to write comments.
Co-authored-by: Simon Høxbro Hansen <[email protected]>
Okay it's now all tidy~ |
Looks much better! However, one test is failing. Can you take a look at it before I review it? |
It was passing locally :| but anyways fixed |
Closes #6266