etesync / etesync-web

An EteSync web client
https://www.etesync.com
GNU Affero General Public License v3.0
247 stars 31 forks source link

Tasks: adds filter by tasks due today #95

Closed AbleLincoln closed 4 years ago

AbleLincoln commented 4 years ago

Yeah I was just being lazy, I'll take care of the TODOs

What do you mean by "filter by showCompleted before passing to the sidebar"?

tasn commented 4 years ago

What do you mean by "filter by showCompleted before passing to the sidebar"?

nvm, my bad.

AbleLincoln commented 4 years ago

This memoization is getting to be a bit more complicated than I had anticipated. And it may require some changes that will affect other parts of the app. If I remove the comments like you suggested, can we merge this in for now (performance is fine, just not optimized), and we can discuss memoization in a separate PR?

tasn commented 4 years ago

Yeah, no problem. The only reason why I suggested memoizing it is because I thought (isn't it?) that it's a matter of changing:

   const amountDueToday = tasks.filter((x) => x.dueDate && moment(x.dueDate.toJSDate()).isSame(moment(), 'day')).length;

to

   const daysSinceEpoch = Math.floor((new Date()).getTime() / 1000 / 60 / 60);
   const amountDueToday = React.useMemo(() => (tasks.filter((x) => x.dueDate && moment(x.dueDate.toJSDate()).isSame(moment(), 'day')).length), [tasks, daysSinceEpoch]);

Admittedly, it was more complex than I anticipated because of the daysSinceEpoch dependency, though I think it should be simpler for the other one that only depends on tasks.

I'm happy to just merge as is, but if you would rather try out the above, maybe we can wait for the memoization?

AbleLincoln commented 4 years ago

I thought it would be that simple too, but the thing is tasks doesn't maintain referential equality (which breaks the memoization). It changes on every re-render caused by Redux. I actually had to go multiple levels up to solve this problem. Check this out:

https://github.com/etesync/etesync-web/blob/02ac73eb25c953c7df1dec74b3f0587eab44b12c/src/Pim/index.tsx#L324-L326

This objValues function is recalculated on every render, causing the resulting prop to lose referential equality. The resulting tasks array is different on every render. So any "memoization" we are doing down the component tree won't even matter (if it depends on tasks).

I have a possible fix for this, if you want me to push it. But like I said, now I'm touching other parts of the app.

tasn commented 4 years ago

Ah damn! I don't think Pim is re-rendered on every call to the store, but rather on every page navigation due to the history prop. The rest should be static, I think.

Either way, yeah, good catch! Please open a PR to fix memoization both here and there.

Thanks!

tasn commented 4 years ago

Hm... I just played with it a bit more, what do you think about not showing anything when the number is 0? So if you have no tasks that are due today, just don't show any number? (same of all the filters).

Another thing actually: I think that "Due Today" should also show tasks that are due before today (so overdue), because it's meant to represent what needs to be done today, and not just what happens to be planned for today. I guess it should show all of the tasks that are due before 00:00 tomorrow.

AbleLincoln commented 4 years ago

Sure I can make those changes

tasn commented 4 years ago

Great, thanks!