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.

Sign up for GitHub

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

Proper messages for unchecking and unstarring items #51

Merged
merged 7 commits into from
Sep 2, 2018
Merged

Proper messages for unchecking and unstarring items #51

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d50b2db
proper messages for unstarring and unchecking
ashinzekene Aug 4, 2018
ff1b2af
fixed linting errors
ashinzekene Aug 4, 2018
f692546
resolved some issues
ashinzekene Aug 19, 2018
abfbb8f
used ternary in for loop
ashinzekene Aug 29, 2018
03f383e
added checks for empty id ary for markIncomplete, markComplete, mark…
ashinzekene Aug 29, 2018
0136768
fixed ordering of checked and unchecked items
ashinzekene Sep 2, 2018
0d91317
Merge branch 'master' into master
klaudiosinani Sep 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions lib/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,17 +213,41 @@ class Render {
}

markComplete(ids) {
if (ids.length === 0) {
return;
}
klaudiosinani marked this conversation as resolved.
Show resolved Hide resolved
const [prefix, suffix] = ['\n', grey(ids.join(', '))];
const message = `Checked ${ids.length > 1 ? 'tasks' : 'task'}:`;
success({prefix, message, suffix});
}

markIncomplete(ids) {
if (ids.length === 0) {
return;
}
const [prefix, suffix] = ['\n', grey(ids.join(', '))];
Copy link
Owner

Choose a reason for hiding this comment

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

It is possible to check if the ids array is empty from within the method, so we can directly call it later on, without having to wrap it inside an if statement:

  markIncomplete(ids) {
+  if (ids.length === 0) {
+    return;
+  }
  const [prefix, suffix] = ['\n', grey(ids.join(', '))];
  const message = `Unchecked ${ids.length > 1 ? 'tasks' : 'task'}:`;
  success({prefix, message, suffix});
}

The same can be done for the markComplete, markStarred & markUnstarred methods.

Copy link

Choose a reason for hiding this comment

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

I think this condition check is not necessary.

Copy link

Choose a reason for hiding this comment

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

The if statement is necessary, @ashinzekene should add them

const message = `Unchecked ${ids.length > 1 ? 'tasks' : 'task'}:`;
success({prefix, message, suffix});
}
klaudiosinani marked this conversation as resolved.
Show resolved Hide resolved

markStarred(ids) {
if (ids.length === 0) {
return;
}
klaudiosinani marked this conversation as resolved.
Show resolved Hide resolved
const [prefix, suffix] = ['\n', grey(ids.join(', '))];
const message = `Starred ${ids.length > 1 ? 'items' : 'item'}:`;
success({prefix, message, suffix});
}

markUnstarred(ids) {
if (ids.length === 0) {
return;
}
const [prefix, suffix] = ['\n', grey(ids.join(', '))];
const message = `Unstarred ${ids.length > 1 ? 'items' : 'item'}:`;
success({prefix, message, suffix});
}
klaudiosinani marked this conversation as resolved.
Show resolved Hide resolved

missingBoards() {
const prefix = '\n';
const message = 'No boards were given as input';
Expand Down
10 changes: 8 additions & 2 deletions lib/taskbook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,13 +307,16 @@ class Taskbook {
checkTasks(ids) {
ids = this._validateIDs(ids);
const {_data} = this;
const [checked, unChecked] = [[], []];
Copy link
Owner

Choose a reason for hiding this comment

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

It is completely Ok to avoid the camel-case here and go for unchecked as a variable name:

const [checked, unchecked] = [[], []];


ids.forEach(id => {
_data[id].isComplete = !_data[id].isComplete;
Copy link
Owner

Choose a reason for hiding this comment

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

Here we can go for a ternary operator as the conditional expression:

ids.forEach(id => {
   _data[id].isComplete = !_data[id].isComplete;
   return _data[id].isComplete ? complete.push(id) : incomplete.push(id);
});

Copy link

Choose a reason for hiding this comment

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

This part has not been updated yet, i can help out if needed

return _data[id].isComplete ? unChecked.push(id) : checked.push(id);
Copy link
Owner

@klaudiosinani klaudiosinani Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Items whose isComplete attribute is updated to true should be placed in the checked array and items whose isComplete attribute is updated to false should be placed in the unchecked one:

return _data[id].isComplete ? checked.push(id) : unchecked.push(id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I think it is right this way. The line above has already changed the status of the tasks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

render.markIncomplete(checked);
and then this

Copy link
Owner

@klaudiosinani klaudiosinani Aug 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The process you are describing is indeed identical, but these arrays, checked[] & unchecked[], are meant to store item IDs based on each item's updated isComplete attribute value, and not on the original/database-stored one, thus reversing them will lead to a clearer and more obvious behavior for future readers. For example: an item stored in the database with its isComplete attribute set to false (unchecked), once selected & checked by the user, will have its value updated to its opposite true (checked), thus its ID should be passed to the checked array. The same holds for the opposite case, where the item is initially marked as complete.

});

this._save(_data);
render.markComplete(ids);
render.markIncomplete(checked);
render.markComplete(unChecked);
Copy link
Owner

Choose a reason for hiding this comment

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

Checked items should be displayed before the unchecked ones, just like with the display of the starred/unstarred items on line 476:

 render.markComplete(checked);
 render.markIncomplete(unchecked);

}

createTask(desc) {
Expand Down Expand Up @@ -462,13 +465,16 @@ class Taskbook {
starItems(ids) {
ids = this._validateIDs(ids);
const {_data} = this;
const [starred, unStarred] = [[], []];
Copy link
Owner

Choose a reason for hiding this comment

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

The camel-case can be dropped here as well:

const [starred, unstarred] = [[], []];


ids.forEach(id => {
_data[id].isStarred = !_data[id].isStarred;
return _data[id].isStarred ? unStarred.push(id) : starred.push(id);
Copy link
Owner

Choose a reason for hiding this comment

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

return _data[id].isStarred ? starred.push(id) : unstarred.push(id);

});
Copy link
Owner

Choose a reason for hiding this comment

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

ids.forEach(id => {
  _data[id].isStarred = !_data[id].isStarred;
  return _data[id].isStarred ? starred.push(id) : unstarred.push(id);
});

Copy link

Choose a reason for hiding this comment

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

This part still hasnt been updated, please let me know if you need help, happy to contribute


this._save(_data);
render.markStarred(ids);
render.markStarred(unStarred);
render.markUnstarred(starred);
Copy link
Owner

Choose a reason for hiding this comment

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

render.markStarred(starred);
render.markUnstarred(unstarred);

}

updatePriority(input) {
Expand Down