fix: optimize condition editing logic in DatasetsFilterSettingsComponent #1673
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Junjiequan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Removing ChangeDetectionStrategy.OnPush might fix the immediate issue but could lead to performance problems. Consider investigating why OnPush was causing problems and fixing that instead.
- Please provide more detailed information in the PR description, particularly in the 'Fixes' and 'Changes' sections, to help reviewers understand the scope and impact of the changes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/app/datasets/datasets-filter/settings/datasets-filter-settings.component.ts
Show resolved
Hide resolved
Junjiequan
force-pushed
the
filter-ui-fix
branch
from
9ab0764
to
c703e3c
Compare
| (config) => isEqual(config.condition, data), | ||
| ); | ||
| if (existingConditionIndex !== -1) { | ||
| console.log("Condition already exists"); |
There was a problem hiding this comment.
Do we need to console.log here or we just want to return if the condition is met?
There was a problem hiding this comment.
Thanks for pointing that out.
Ideally, we should show a snack bar popup with the msg to let the user know what's happening, but I've noticed that we have disabled the snack bar on staging and production.
I've kept the waning in a console.log, in case we want to improve the snack bar popup in the future.
There was a problem hiding this comment.
Maybe it is a good idea to try to use the snackbar and try to communicate with the user if possible instead of just console.log it silently. Aren't we able to fire the snackbar and add a message even if other notifications are disabled for requests in the interceptor?
There was a problem hiding this comment.
Aha, good point. Maybe we can import the snackbar module within the component, instead of relying on the global snackbar
Junjiequan
force-pushed
the
filter-ui-fix
branch
from
6203644
to
1501014
Compare
Description
This PR addresses the following issues:
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Optimize the condition editing logic in the DatasetsFilterSettingsComponent to fix UI issues related to scientific metadata conditions and input value changes.
Bug Fixes: