Skip to content
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

[#1749] Fixed activeFilterCount #1747

Merged
merged 6 commits into from
May 11, 2021
Merged

Conversation

stayprodio
Copy link
Contributor

@stayprodio stayprodio commented May 6, 2021

closes #1749

@stayprodio stayprodio requested a review from AudreyKj May 6, 2021 14:14
@github-actions github-actions bot added the fix label May 6, 2021
Copy link
Contributor

@lucapette lucapette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand the code correctly:

  • we do not need the count (it's either "active" or not the filter)
  • if that's true, then the variable keeping the count is not needed as you can do one long or expression like filterA.length > 0 || filterB.length >0 and so on which would declutter the function quite a bit
  • if we do need the count, then I'd declare the variable right before using it inside the function and also rename it to activeFilterCount(): number so it's much clearer what it does (the little details you know :))
  • Whatever we need the count or not, I'm not sure I understand why sometimes we write currentFilter?. and others we do currentFilter.. As I see it, either it's always needed or never needed (maybe I'm not getting it though). If it's always needed I'd early return currrentFilter === undefined then false (or 0 if we need the count)

@stayprodio
Copy link
Contributor Author

Unfortunately we need to count as we have 3 states for isStateOpen (true, false and undefined) so currentFilter.length is 1 although we didn't set any filter when we select "ALL". DisplayName has kinda the same behaviour, if you never search it never appears in currentFilter, but after you searched and you removed it it appears as null or undefined in currentFilters.

AitorAlgorta
AitorAlgorta previously approved these changes May 7, 2021
@stayprodio stayprodio dismissed lucapette’s stale review May 11, 2021 09:16

all changes made

@stayprodio stayprodio merged commit 27bc5a7 into develop May 11, 2021
@stayprodio stayprodio deleted the fix/1749-fix-activeFilterCount branch May 11, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants