-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: Filter hitArea not updating #10530
Conversation
My review is in progress 📖 - I will have feedback for you in a few minutes! |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cb418be:
|
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.
I have reviewed your code and found 1 potential issue. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
|
||
// Clone the filters array so we don't freeze the user-input | ||
value = Array.isArray(value) ? value.slice(0) : value; | ||
|
||
// Ensure filters are immutable via filters getter | ||
this._filters.filters = Object.freeze(value); | ||
effect.filters = Object.freeze(value); |
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.
The code is directly mutating the 'filters' property of the FilterEffect instance. While this might not cause any immediate issues, it's generally a good practice to avoid direct mutation of object properties. Instead, consider using a method to update the property which can also handle any necessary side effects or validations.
When this lands will it be possible to dynamically change In my case I have a For now the |
* simplify masking * tidy
Description of change
This PR simplifies the filter mixin a little bit. In the process it also fixes #10524.
Pre-Merge Checklist
npm run lint
)npm run test
)