-
Notifications
You must be signed in to change notification settings - Fork 35
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
Respect date filter in event list for multi day events #2007
Conversation
groups={groups} | ||
groups={groups.filter((group) => { | ||
const groupDateToMoment = moment(group.date); | ||
const dateFilter = currentSearch.advancedSearch?.dates?.start; |
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.
does this view only get rendered in search?
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.
Gets rendered in Combined, Planning only and Event only views
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.
Only in search mode? You're filtering out items that don't have some search filters. What if user isn't in search mode?
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.
If user isn't in search mode the filter would be pointless. Good point, I'll add a check beforehand so we don't iterate needlessly, we can pass the items right away
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.
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.
Nope, date filter has a default of today, so in theory, it's never null
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 see. Still, no filter should mean no filtering should be happening. Which means all items should be returned. Even if all existing code paths return the same result before/after, your latest approach is still better.
There's a good rule of least surprise that I haven't had a chance to mention to you yet that applies here.
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.
@tomaskikutis added a comment & pushed changes |
SDESK-7310