-
Notifications
You must be signed in to change notification settings - Fork 310
Implement due dates for cards #189
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
Conversation
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Codecov Report
@@ Coverage Diff @@
## master #189 +/- ##
==========================================
+ Coverage 76.12% 76.56% +0.44%
==========================================
Files 33 33
Lines 955 973 +18
==========================================
+ Hits 727 745 +18
Misses 228 228 |
artemanufrij
left a comment
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.
Colors should be changed as discussed in IRC...
| app.filter('parseDate', function() { | ||
| return function (date) { | ||
| if(moment(date).isValid()) { | ||
| return moment(date).format('YYYY-MM-DD'); |
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.
Shouldn't format be based on Nextcloud locale? I know it's a hot topic right now, since Nextcloud fetches locale based on language settings, but still - something to keep in mind
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.
Yes, but that is out of scope for now, since we want to release 0.2 soon. We should also be timezone aware. But let's address that afterwords.
templates/main.php
Outdated
| Util::addScript('deck', 'vendor/jquery-timepicker/jquery.ui.timepicker'); | ||
|
|
||
| if(!\OC::$server->getConfig()->getSystemValue('debug', false)) { | ||
| if(true && !\OC::$server->getConfig()->getSystemValue('debug', false)) { |
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.
why is this change needed?
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.
🙈 Some debugging statement I forgot about.
templates/part.card.php
Outdated
| <div id="duedate"> | ||
| <input class="datepicker-input medium focus" type="text" placeholder="Set a due date" value="{{ cardservice.getCurrent().duedate | parseDate }}" datepicker="due" /> | ||
| <input class="timepicker-input medium focus" type="text" placeholder="00:00:00" ng-if="cardservice.getCurrent().duedate" value="{{ cardservice.getCurrent().duedate | parseTime }}" timepicker="due" /> | ||
| <button class="icon icon-delete button-inline" title="Remove duedate" ng-if="cardservice.getCurrent().duedate" ng-click="resetDuedate()"></button> |
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.
title should be translatable string. Also, missing space between "due" and "date"
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.
👍
img/calendar.svg
Outdated
| @@ -0,0 +1 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" height="32" width="32" viewbox="0 0 32 32"><path d="M8 2c-1.108 0-2 .892-2 2v4c0 1.108.892 2 2 2s2-.892 2-2V4c0-1.108-.892-2-2-2zm16 0c-1.108 0-2 .892-2 2v4c0 1.108.892 2 2 2s2-.892 2-2V4c0-1.108-.892-2-2-2zM11 6v2c0 1.662-1.338 3-3 3S5 9.662 5 8V6.125A3.993 3.993 0 0 0 2 10v16c0 2.216 1.784 4 4 4h20c2.216 0 4-1.784 4-4V10a3.993 3.993 0 0 0-3-3.875V8c0 1.662-1.338 3-3 3s-3-1.338-3-3V6zM6.094 16h19.812a.09.09 0 0 1 .094.094v9.812a.09.09 0 0 1-.094.094H6.094A.09.09 0 0 1 6 25.906v-9.812A.09.09 0 0 1 6.094 16z"/></svg> | |||
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 file is available in core/img/places
img/calendar-white.svg
Outdated
| @@ -0,0 +1,54 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
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've created a PR to add white calendar icon to core in nextcloud/server#5447 - so this should become redundant
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.
👍 But we should keep it for now until all supported Nextcloud versions ship with that icon.
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 agree, but please use the optimized version - either copy from my PR or run scour command:
scour -i calendar-white.svg -o calendar-white-new.svg --create-groups --enable-id-stripping --enable-comment-stripping --shorten-ids --remove-metadata --strip-xml-prolog --no-line-breaks
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.
done :)
css/style.css
Outdated
| } | ||
| .due.now { | ||
| background-color: #fbd850; | ||
| color: #000; |
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.
not needed - black is default
css/style.css
Outdated
| color: #fff; | ||
| } | ||
|
|
||
| .due .icon-calendar { |
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.
you should move this over to .due .icon as it is a default setting. Other .something .icon-calendar declarations will override background-image and margin-right is needed for all.
css/style.css
Outdated
| } | ||
|
|
||
| #card-dates span { | ||
| #card-meta #duedate { |
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's usually not a good idea to used ID as a CSS selector. I would recommend adding a class and applying style to it.
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
|
@pixelipo @artemanufrij Thanks for the comments, should be all fixed now. |
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
artemanufrij
left a comment
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.
now it looks good for me...
Signed-off-by: Julius Härtl <jus@bitgrid.net>
|
Thanks @artemanufrij Let's merge this. 🚀 |
|
Nice work! 🎉 |


Fix for #106
cc @artemanufrij @pixelipo