Adds some initial improvements to Tasks #74

AbleLincoln · 2020-01-17T20:32:28Z

This is the MVP for a few features I want to add to Tasks. The code is by no means good or clean yet, I just wanted to get out a few rough ideas first. Let me know what you think of the following. Anything can be changed.

I've added:

quick input right from the list screen
tags: They show up next to tasks and can be added via the edit form. The textbox currently accepts space-separated values for the tags. I would like to improve this input method.
edit form cleanup: there were a lot of fields that I felt were extraneous (location, timezone, start date) so I have removed them. Let me know if you actually want to keep any of these
better datetimepicker. Material UI has a pretty good component for this that we were not using, so I have dropped theirs in.

tasn · 2020-01-18T18:23:46Z

src/pim-types.ts

+  }
+
+  get tags() {
+    return JSON.parse(this.component.getFirstPropertyValue('tags'));


I'll properly review everything slightly later, when you make more progress, but one important comment: this shouldn't be called "tags" but "properties", and you shouldn't JSON.stringfiy it, but rather it should be an array of strings.

This is not something that comes from me, it comes from the ical standard. This object that you're updating is an ical object that then needs to be serialized to the ical format. It doesn't change much, just make sure to rename it and change the value. Additional properties of tags, such as colour, should be saved elsewhere, and I'll happily explain it when you get to saving those. Though those should be maintained at a per-journal capacity as part of the journal metadata.

I see. I think I've got it now. It was a little confusing to figure out how to do multi value properties, but looks like I need to explicitly add the "CATEGORIES" property to the TaskType.

Additional properties of tags, such as colour, should be saved elsewhere, and I'll happily explain it when you get to saving those. Though those should be maintained at a per-journal capacity as part of the journal metadata.

Could I also save a list of all the tags this way? If so, how would I go about that?

The list of tags is external to specific tasks. It should probably be stored in each journal's metadata. I'm happy to explain to you how to do it (it would require a tiny bit of code on my side), though I think it's too early for that, just save it locally (and globally) for now, or heck, make the tags hardcoded while you are working on it, and ping me again in a few days / a week and I'll add the ability to set generic journal metada.

src/components/TaskList.tsx

package.json

src/components/TaskEdit.tsx

@tasn

Note by @tasn: I actually wrote this from scratch, but slightly based it on work by @AbleLincoln in #74 which is why I set him as the author. Fixes #9

tasn · 2020-02-14T11:31:23Z

Hey,

Just one general comment, if you fix a general issue with the code, please have it in a separate PR and at the very least a separate commit in a PR so it can be easily merged. I just fixed #9, and had to implement it from scratch because of this. A fix that could have been in for a while now... :)

Additionally, your fix was incorrect because it was ignoring the "dateOnly" prop.

better datetimepicker

Adds UI features like toolbar (w/ sorting & searching), sidebar (w/ filtering), light editing improvements. Adds task type features like priority, tags. Adds some use of Context for prop drilling. Adds Task Settings to the redux store. This is a massive commit. I wanted to keep everything separate but the code is too tightly coupled.

AbleLincoln · 2020-02-22T22:23:09Z

I've added a bunch of features:

Task properties

tags: currently tags are hard-coded, we have "work" and "home". I will add dynamic tags in a follow-up PR, it shouldn't be too hard
priority: none, low, med, high

Task list

colored check boxes corresponding to priority
tags
secondary text displaying how long until the task is due

Sidebar

Basically where all of the filtering happens. Currently you can filter by all, due today, and tag

Toolbar

Contains UI for sorting, searching, and toggling show completed. Currently you can sort by priority, due date, title, last modified.

Context

I am using a bit of React Context to pass down props for saving and selecting tasks

Redux Store

The store now holds settings for the task page. This allows the user to maintain the filters, sorts, etc. between page reloads.

tasn

Hey,

Good job, and glad to see things are making progress! Can't wait to have it in!

I made a few inline comments in the code, suggesting some changes that didn't look right to me, though I do have some more general comments I'd like to make.

It was very hard to review this, and I'm sure my work reviewing it is not done. It's not about the code itself, but rather than you changed 20 files at once, with some changes being unrelated to what this PR this about. In addition, because you haven't split the changes to different commits, it was a lot of context to manage in my head while reviewing.
Here's just an example split to commits that you could have done (commit messages are bad, and are just to convey the idea):

"Remove the task viewing page and make it always go to edit"
"Redesign the task list page"
"Add a task bar to the task list page"
"Add ColorRadio widget for having coloured radio boxes"
"Redesign the task edit page"
...

This would have made it much easier to review, because then related changes would be grouped together, and I wouldn't have to keep on scrolling through walls of source code to try to understand what is related to what.

I know you are new to open source, so really no worries (and no harm done!), though please try reorganising things a bit so it's easier to review your code. Because the whole point of code review is to be able to find issues and discuss choices (and tradeoffs), and it's really hard with suche large changes.

Looking forward to the next review iteration!

tsconfig.json

src/types/ical.js.d.ts

tasn · 2020-02-24T14:09:31Z

src/store/reducers.ts

+
+    tasks: {
+      showCompleted: false,
+      sortOrder: 'dueDate',


I'm not sure if you took a look at the code I sent you back then, but I assume you have some sort of a "smart" sort, that should be the default, not this.

tasn · 2020-02-24T14:12:49Z

src/store/reducers.ts

+  showCompleted: boolean;
+  sortOrder: string;
+  filterBy: string;
+  searchTerm: string;


This should not be a setting, it's ephemeral, not persistent... It should just be a prop / navigation property.

The reason I have these as settings is that I want the filtering/sorting to persist when the user navigates away from the Tasks page. The main use case is when a user goes to edit/create a task. After saving and navigating back to the tasks page, all the sorting/filtering is reset, which can be frustrating.

Do you have a suggestion for another way I can handle this?

Sorry if my initial comment wasn't clear, I was referring specifically to searchTerm, not the rest. The rest should indeed be as you did them. As I told you elsewhere, it's similar to how I did it in etesync-tasks, which you can refer to if you want a glimpse into my thoughts process on things, and anyhow, a lot of it is very similar. Obviously my thoughts aren't necessarily good, but at least you can see what I mean. :)

src/Pim/index.tsx

src/components/Tasks/TaskEdit.tsx

src/components/Tasks/TaskListItem.tsx

src/pim-types.ts

src/store/actions.ts

tasn · 2020-02-28T12:50:35Z

One more related comment to save both of us a lot of time: maybe let's do things step by step? So maybe have a PR that I can merge that changes the things that are not really related to this PR? And then have another small PR that does X and I can already merge, and so on? So we at least know things are good and can move forward? Only if it makes things easier for you...

What I'm essentially trying to do is shorten the feedback loop, so you can change things and get immediate feedback rather than sitting on your changes for a few days, waiting for my feedback, and then sitting on your changes again for a few days.

AbleLincoln · 2020-02-28T21:13:49Z

Yes I think that is a good idea. What would be the best way to go about this regarding git/GitHub since I already have a lot of these changes in progress on this branch. I'm sorry, I'm just not too experienced with complicated merges.

tasn · 2020-02-28T21:21:51Z

Yes I think that is a good idea. What would be the best way to go about this regarding git/GitHub since I already have a lot of these changes in progress on this branch.

Great, so I'm looking forward to your smaller PRs! Just think what changes make sense standalone and open the appropriate PRs. The added benefit is that we can discuss separate topics separately. So for example, we will be able to discuss search in one and quick-add in another.

I'm sorry, I'm just not too experienced with complicated merges.

Don't be silly, nothing to be sorry about! I didn't mean my previous messages as complaints, I meant them as guiding. I know it's new for you, and I'm here to help! I was just telling you what I think could make things easier based on my experience.

AbleLincoln · 2020-02-28T22:17:24Z

Thank you for all your help and understanding. Do you recommend I just create completely new branches from the master and copy over changes?

tasn · 2020-02-28T22:34:57Z

Yeah, maybe just creating new branches and copying over changes is the easiest way to go. It's likely that some of your PRs will rely on others, which is fine too. Even within PRs, split to commits as it makes sense to you (maybe no split is needed), so unrelated code isn't grouped together.

tasn · 2020-03-03T07:02:01Z

Hey,

Just one thing I noticed now that we have merged #81: you can't access the change history anymore. That link should be added to the task edit page. Please keep that in mind.

AbleLincoln · 2020-03-05T23:50:34Z

Just one thing I noticed now that we have merged #81: you can't access the change history anymore. That link should be added to the task edit page. Please keep that in mind.

Noted, thank you for the heads up. I will add this in

tasn · 2020-03-06T08:23:17Z

Another comment that comes to mind: the mapping of High, Medium and Low priority from the UI to ical should be:
High: 1-4
Medium: 5
Low: 6-9

I took a look at the RFC and that's the mapping they used there. Just FYI.

tasn · 2020-03-09T09:52:33Z

Another thing I noticed, especially given that we now have quickadd: we need a way to choose the default task list - so it'll both be the one in the new event dialog, but also the one added to from quickadd.

Edit: just to clarify, it doesn't need to happen now, but just so we remember it for later on.

AbleLincoln · 2020-04-05T22:19:00Z

closing because all of these features have now been implemented in other PRs

tasn · 2020-04-06T10:35:33Z

\o/

AbleLincoln requested a review from tasn January 17, 2020 20:32

tasn reviewed Jan 18, 2020

View reviewed changes

src/components/TaskList.tsx Outdated Show resolved Hide resolved

tasn reviewed Jan 18, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

tasn reviewed Jan 20, 2020

View reviewed changes

src/components/TaskEdit.tsx Outdated Show resolved Hide resolved

tasn pushed a commit that referenced this pull request Feb 14, 2020

DateTimePicker: change to a more usable date and time picker

399724f

Note by @tasn: I actually wrote this from scratch, but slightly based it on work by @AbleLincoln in #74 which is why I set him as the author. Fixes #9

AbleLincoln added 4 commits February 15, 2020 12:37

Adds tags, cleans up edit form,

bb54d57

better datetimepicker

Implements ical's "categories" for task tags

a21dbba

Replaces DateTimePicker's date-fns dep with moment

0bdc0e2

AbleLincoln force-pushed the feature/tasks branch from 8b5463f to 0907a47 Compare February 22, 2020 22:13

AbleLincoln requested a review from tasn February 22, 2020 22:16

tasn requested changes Feb 24, 2020

View reviewed changes

AbleLincoln mentioned this pull request Mar 14, 2020

Tasks: adds tags functionality #92

Merged

AbleLincoln closed this Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds some initial improvements to Tasks #74

Adds some initial improvements to Tasks #74

AbleLincoln commented Jan 17, 2020

tasn Jan 18, 2020

AbleLincoln Jan 20, 2020

AbleLincoln Jan 20, 2020

tasn Jan 20, 2020

tasn commented Feb 14, 2020

AbleLincoln commented Feb 22, 2020

tasn left a comment

tasn Feb 24, 2020

tasn Feb 24, 2020

AbleLincoln Feb 28, 2020

tasn Feb 28, 2020

tasn commented Feb 28, 2020

AbleLincoln commented Feb 28, 2020

tasn commented Feb 28, 2020

AbleLincoln commented Feb 28, 2020

tasn commented Feb 28, 2020

tasn commented Mar 3, 2020

AbleLincoln commented Mar 5, 2020

tasn commented Mar 6, 2020

tasn commented Mar 9, 2020 •

edited

AbleLincoln commented Apr 5, 2020

tasn commented Apr 6, 2020

Adds some initial improvements to Tasks #74

Adds some initial improvements to Tasks #74

Conversation

AbleLincoln commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasn commented Feb 14, 2020

AbleLincoln commented Feb 22, 2020

Task properties

Task list

Sidebar

Toolbar

Context

Redux Store

tasn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tasn commented Feb 28, 2020

AbleLincoln commented Feb 28, 2020

tasn commented Feb 28, 2020

AbleLincoln commented Feb 28, 2020

tasn commented Feb 28, 2020

tasn commented Mar 3, 2020

AbleLincoln commented Mar 5, 2020

tasn commented Mar 6, 2020

tasn commented Mar 9, 2020 • edited

AbleLincoln commented Apr 5, 2020

tasn commented Apr 6, 2020

tasn commented Mar 9, 2020 •

edited