-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
fix: markArea display filter correction #16861
Conversation
Thanks for your contribution! |
src/component/marker/MarkAreaView.ts
Outdated
@@ -123,6 +123,20 @@ function markAreaFilter(coordSys: CoordinateSystem, item: MarkAreaMergedItemOpti | |||
) { | |||
return true; | |||
} | |||
/*Another zone filter is applied, which might not be necessary. | |||
Directly returning true means displaying markArea component permanently, |
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 prefer using inline comment // here.
Also this comment seems to be outdated after new commits?
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.
For this one, I mean we are using a function zonefilter
here, but actually returning true can achieve almost the same effect as using this function. The reason is that, when returning true, markArea
is always shown and will not be clipped however the grid changes, which solves the problem. When the coordinates does not include the markArea
, it's automatically not shown. So in my opinion a function to check if the coordinates intersect with markArea
is unnecessary. Of course, keeping the markArea
rendered may be problematic when there are huge amount of them, but in most cases one or two markArea
s would hardly affect performance.
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.
Got it. Thanks for the detailed explanation. Perhaps we can tweak the description a bit to make it more clear. For example:
Directly returning true may also do the work. But filtering ahead will avoid creating too much unnecessary elements when there are huge amount of elements.
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.
Good point. Comment updated.
Added recorded user interaction for visual regression testing |
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
Brief Information
This pull request is in the type of:
What does this PR do?
Correct the markArea filter when both diagonal points are not in coordinate system.
Fixed issues
Details
Before: What was the problem?
MarkArea will disappear if both anchor points of it are not in the coordinate system.
After: How is it fixed in this PR?
MarkArea is correctly displayed.
Misc
Related test cases or examples to use the new APIs
Attached