-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
Proper messages for unchecking and unstarring items #51
Conversation
lib/render.js
Outdated
@@ -218,12 +218,24 @@ class Render { | |||
success({prefix, message, suffix}); | |||
} | |||
|
|||
markIncomplete(ids) { | |||
const [prefix, suffix] = ['\n', grey(ids.join(', '))]; |
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.
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.
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 think this condition check is not necessary.
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.
The if statement is necessary, @ashinzekene should add them
lib/taskbook.js
Outdated
@@ -307,13 +307,25 @@ class Taskbook { | |||
checkTasks(ids) { | |||
ids = this._validateIDs(ids); | |||
const {_data} = this; | |||
const checked = []; | |||
const unChecked = []; |
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.
It is possible to group the assignments here, using destructuring, in order to keep the method as tidy as possible:
const [complete, incomplete] = [[], []];
lib/taskbook.js
Outdated
checked.push(id); | ||
} else { | ||
unChecked.push(id); | ||
} | ||
_data[id].isComplete = !_data[id].isComplete; |
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.
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);
});
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.
This part has not been updated yet, i can help out if needed
lib/taskbook.js
Outdated
} | ||
if (unChecked.length > 0) { | ||
render.markComplete(unChecked); | ||
} |
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.
The if
statements can be dropped here, and directly call the methods:
render.markComplete(complete);
render.markIncomplete(incomplete);
lib/taskbook.js
Outdated
@@ -462,13 +474,25 @@ class Taskbook { | |||
starItems(ids) { | |||
ids = this._validateIDs(ids); | |||
const {_data} = this; | |||
const starred = []; | |||
const unStarred = []; |
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.
For the same reasons mentioned above:
const [starred, unstarred] = [[], []];
lib/taskbook.js
Outdated
starred.push(id); | ||
} else { | ||
unStarred.push(id); | ||
} | ||
_data[id].isStarred = !_data[id].isStarred; | ||
}); |
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.
ids.forEach(id => {
_data[id].isStarred = !_data[id].isStarred;
return _data[id].isStarred ? starred.push(id) : unstarred.push(id);
});
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.
This part still hasnt been updated, please let me know if you need help, happy to contribute
lib/taskbook.js
Outdated
} | ||
if (starred.length > 0) { | ||
render.markUnstarred(starred); | ||
} |
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.
render.markStarred(starred);
render.markUnstarred(unstarred);
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.
Looks good! A couple of changes and it is good to go. Please, feel free to share your thoughts on the proposed modifications : )
…tarred & markUnstarred methods.
Sorry I have not replied in a while. I'm working on my undergraduate project and I'm far behind schedule. |
lib/taskbook.js
Outdated
@@ -307,13 +307,16 @@ class Taskbook { | |||
checkTasks(ids) { | |||
ids = this._validateIDs(ids); | |||
const {_data} = this; | |||
const [checked, unChecked] = [[], []]; |
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.
It is completely Ok to avoid the camel-case here and go for unchecked
as a variable name:
const [checked, unchecked] = [[], []];
lib/taskbook.js
Outdated
|
||
ids.forEach(id => { | ||
_data[id].isComplete = !_data[id].isComplete; | ||
return _data[id].isComplete ? unChecked.push(id) : checked.push(id); |
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.
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);
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.
No. I think it is right this way. The line above has already changed the status of the tasks
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.
render.markIncomplete(checked);
and then this
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.
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.
lib/taskbook.js
Outdated
}); | ||
|
||
this._save(_data); | ||
render.markComplete(ids); | ||
render.markIncomplete(checked); | ||
render.markComplete(unChecked); |
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.
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);
lib/taskbook.js
Outdated
@@ -462,13 +465,16 @@ class Taskbook { | |||
starItems(ids) { | |||
ids = this._validateIDs(ids); | |||
const {_data} = this; | |||
const [starred, unStarred] = [[], []]; |
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.
The camel-case can be dropped here as well:
const [starred, unstarred] = [[], []];
lib/taskbook.js
Outdated
|
||
ids.forEach(id => { | ||
_data[id].isStarred = !_data[id].isStarred; | ||
return _data[id].isStarred ? unStarred.push(id) : starred.push(id); |
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.
return _data[id].isStarred ? starred.push(id) : unstarred.push(id);
lib/taskbook.js
Outdated
}); | ||
|
||
this._save(_data); | ||
render.markStarred(ids); | ||
render.markStarred(unStarred); | ||
render.markUnstarred(starred); |
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.
render.markStarred(starred);
render.markUnstarred(unstarred);
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.
Looks very good! A couple of minor updates and it is good to be merged : )
I think we are good now |
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.
Looks great! Thank you a lot for taking the time to contribute : )
Fixes #27
Shows the right message when unchecking and unstarring items. For example