Bug Fix: Mutliple filter components overwrite each others conditions when filtering the same grid #7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… event is now fired, telling other filter components that they should add their filter back to the grid.
|
AB-xdev
requested changes
Oct 21, 2024
There was a problem hiding this comment.
Some general problems from my side:
- I'm having some serious doubts if this change is needed in the first place. I think the use-case of needing multiple "filter components" for one Grid is extremely unlikely.
- The current solution is not bound to the Grid that should be filter but instead to the UI, meaning that all "filter components" on the same UI are updated, which can result in unexpected behavior.
- Self-registering an listener for communication between different components is not a good practice and breaks encapsulation. This will result in "works with magic"-behavior observed by users of the component.
| this.registration.remove(); | ||
| } | ||
|
|
||
| private static class FilterComponentResetGridFilterEvent extends ComponentEvent<FilterComponent> |
There was a problem hiding this comment.
Type is missing, see sonar for more details
| this.removeQueryParameter(chip); | ||
| } | ||
|
|
||
| private void updateGridFilter() |
There was a problem hiding this comment.
Method is called updateGridFilter but it just removes filters and throws a FilterComponentResetGridFilterEvent.
Should likely be called resetGridFilters or?
Comment on lines
+1536
to
+1550
| final List<Predicate<T>> predicates = new ArrayList<>(); | ||
|
|
||
| for(final ChipBadge<FilterCondition<T, ?>> chipBadge : chipBadges) | ||
| { | ||
| final FilterCondition<T, ?> item1 = chipBadge.getItem(); | ||
| final String inputValue = item1.getInputValue(); | ||
|
|
||
| predicates.add( | ||
| item1.getSelectedCondition().compare( | ||
| item1.getItem().getValueProvider(), | ||
| inputValue) | ||
| ); | ||
| } | ||
|
|
||
| return predicates.stream().map(p -> p.test(item)).reduce(Boolean::logicalAnd).orElseThrow(); |
There was a problem hiding this comment.
Can likely be written with Streaming API
| } | ||
|
|
||
| // Needed for interacting with other filter components on the same ui | ||
| private Registration registration; |
There was a problem hiding this comment.
Should be protected so that it can be overriden
|
I think @AB-xdev has good points as to why we should not merge this change: #7 (review) |
JohannesRabauer
deleted the
multiple-fcs-overwrite-each-other-conditions-fix
branch
xdev-gh-bot
pushed a commit
that referenced
this pull request
Sep 1, 2025
…action-digest Update lycheeverse/lychee-action digest to 885c65f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Steps to reproduce:
Expected behavior: At step 4 the second condition should not overwrite the first one. The second condition should be combined with the first condition.