AAE-26155 Added feature for refreshing filter #10266
Conversation
tomaszhanaj
requested review from
eromano,
popovicsandras and
DenysVuika
as code owners
tomaszhanaj
force-pushed
the
fix/AAE-26155-notification-badge
branch
2 times, most recently
from
5eedaaa to
c845bb7
Compare
| /** (optional) Toggles showing an icon by the side of each filter */ | ||
| @Input() | ||
| set refreshedFilterKey(value: string[]) { | ||
| if (value?.length) { |
There was a problem hiding this comment.
it's not clear to me what is happening here : We get as input an array. but there is always one element and we delete the set .
1 - I would avoid having operation on the set for an input attributes as this might lead to some hidden bugs.
2 - If we want a value why don't we just accept a value?
There was a problem hiding this comment.
updatedFiltersSet - variable that hold filters that has been updated. If filter like is my-tasks is in this set the number beside label has green background (notification badge).
We receive array with one element to be sure that this setter was triggered. Every time something changes we are creating new array (reference is changed). Passing new reference triggers input property. Passing twice same primitive value will not trigger setter.
For example: It may happen that my-tasks were updated and user refreshed it (badge was removed). But in the meantime it may happen that my-tasks were updated again. And user refreshed it again -> same value was passed to input. If we have input with primitive value (string) setter will not be triggered because value has not changed (but we need remove the notification badge from number). So my-tasks were not removed from updatedFiltersSet and there is notification badge over the number.
There was a problem hiding this comment.
@tomaszhanaj This is exactly my point, why instead of passing an array we pass the plain value as it's the thing that we are interested in? we can filter on the parent component to have it as plain string and have angular do the job of triggering the change when the value ACTUAL changes if what i got is correct
There was a problem hiding this comment.
@VitoAlbano We need to pass values to the component and it may happen that subsequent values will be the same, for example:
my-tasks -> my tasks -> my tasks
And we need to run code inside input setter every time we get a value, even if it's the same as before.
Reason why I used array is to protect against the situation that Angular checks that the value passed to refreshedFilterKey has not changed and therefore does not perform the action.
There was a problem hiding this comment.
Example input setter.zip
| @@ -46,7 +46,7 @@ export abstract class BaseTaskFiltersCloudComponent implements OnDestroy { | |||
| error: EventEmitter<any> = new EventEmitter<any>(); | |||
|
|
|||
| counters$: { [key: string]: Observable<number> } = {}; | |||
| updatedCounters: string[] = []; | |||
| updatedCountersSet = new Set<string>(); | |||
There was a problem hiding this comment.
HAve you switched this to a Set to better handling adding/removing?
There was a problem hiding this comment.
yes, easier adding and removing. and there is no need for checking whether element is already included.
| @@ -32,6 +32,14 @@ import { TaskCloudEngineEvent } from '../../../models/engine-event-cloud.model'; | |||
| encapsulation: ViewEncapsulation.None | |||
| }) | |||
| export class TaskFiltersCloudComponent extends BaseTaskFiltersCloudComponent implements OnInit, OnChanges { | |||
| /** (optional) Toggles showing an icon by the side of each filter */ | |||
| @Input() | |||
| set refreshedFilterKey(value: string[]) { | |||
There was a problem hiding this comment.
this api does not make much sense without explanation
There was a problem hiding this comment.
sorry, I just did copy-paste and did not change it 🤦♂️
it should be updated now
|
I tried to run the branch on local (HFA and ADF) to verify the fix but refresh button was always disabled for me. how can we check it? |
Answered on slack |
I ran the branch on local and it works. thanks |
VitoAlbano
force-pushed
the
fix/AAE-26155-notification-badge
branch
from
d50c50a to
bb4dadc
Compare
tomaszhanaj
force-pushed
the
fix/AAE-26155-notification-badge
branch
from
bb4dadc to
09e9860
Compare
VitoAlbano
force-pushed
the
fix/AAE-26155-notification-badge
branch
from
09e9860 to
b88ab03
Compare
|
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/AAE-26155
What is the new behaviour?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: