-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
obsidian-reminders modal #432
Conversation
Thank you so much @ZibraMax! I am busy right now, so please be patient. |
@ZibraMax I did a test of this PR and it works really well but one thing I notice is that the calendar modal that picks reminder date/time is not appearing when the plugin is running on Obsidian mobile version (Android to be specific). I think it's important to make it work on mobile devices since typing date/time is more inefficient than PCs. So hope it will be addressed. Thx |
Thank you for the update @ZibraMax! I haven't forgotten about you. |
Hi! Sorry for the late reply, college leaves no time. The calendar modal option was a test. It was not intended in the original idea of the reminder option. I think the modal not working on android depends on the android version (mine is 11, and it works fine). But the position of the calendar button changes. As a future improvement, the calendar modal can be achieved by using the calendar plugin ui. This may be the topic of a future PR :) Thanks for your response and your time! |
You weren’t forgotten! I had to cut a release to finally get live preview out of the door. |
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 is really good. Thank you very much indeed for doing it.
A number of the comments below are reminders for me, when I merge this in.
But there are a few questions mixed in, if you have time to look at them.
@@ -1,11 +1,13 @@ | |||
export interface Settings { | |||
globalFilter: string; | |||
removeGlobalFilter: boolean; | |||
reminder: boolean; //To activate the reminder input |
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 will probably rename this to something that removes the need for a comment, perhaps remindersEnabled
or remindersActivated
|
||
containerEl.createEl('div', { | ||
cls: 'setting-item-description', | ||
text: 'Enable reminder on edit/create task (requires obsidian-reminder plugin)', |
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.
Does it actually require the reminder plugin, or is it there is only really any benefit of you are using the reminders plugin?
(By which I mean, do we need to add code to check if the reminders plugin is enabled?)
Is obsidian-reminder
the plugin id? Would it be better to use the plugin name that users will see in the Plugin store?
new Setting(containerEl) | ||
.setName('Enable reminder on edit/creat task') | ||
.setDesc( | ||
'Enable reminder on edit/create task (requires obsidian-reminder plugin)', |
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.
There seems to be some duplicated text here: Enable reminder on edit/create task (requires obsidian-reminder plugin)
I may refactor it out to create a variable for the duplicated text.
}); | ||
|
||
new Setting(containerEl) | ||
.setName('Enable reminder on edit/creat task') |
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.
Typo: creat -> create
@@ -77,6 +80,7 @@ export class Task { | |||
doneDate, | |||
recurrence, | |||
blockLink, | |||
reminderTime, //reminder attribute, same as dueDate but with time |
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 might rename this to reminderDateTime, to make it clear that it includes a date as well. Not sure about that though.
In general, it's good to make the names clear enough that comments are not needed, especially in view of how many times this comment is repeated further down.
// Add a "max runs" failsafe to never end in an endless loop: | ||
const maxRuns = 7; | ||
const maxRuns = 10; |
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.
@ZibraMax Can you remember why you increased maxRuns
please?
@@ -363,6 +384,13 @@ export class Task { | |||
public toString(layoutOptions?: LayoutOptions): string { | |||
layoutOptions = layoutOptions ?? new LayoutOptions(); | |||
let taskString = this.description; | |||
//Reminder can't be inshort mode, otherwise obsidian-reminder won't work!!!! |
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 short mode is only ever used for displaying the text in Tasks results blocks.
When Tasks writes tasks out as Markdown, it always writes all fields.
Therefore, I think a new option to hide reminders should be added, for display purposes. I can do this as a separate pull request though.
}; | ||
//Only load reminder panel if activated in settings | ||
const { reminder } = getSettings(); | ||
if (!reminder) { |
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 !reminder
- why the negation?
Oh, the comment says 'only load' but I think that what the code is doing suggests that the comment should say something like Remove the reminder panel if not activated in settings
Before merging this, I have to figure out how to safely resolve merge conflicts, whilst still retaining the original author identity of the original commits. |
Any update on this PR? I ended up just modifying my installation of the reminder plugin to use the ⏳ emoji instead of ⏰ but would love to have something native with time support no less. Didn't see this on the Tasks (Obsidian plugin) Roadmap so just wanted to check on this. --Edit Looks like the code base has changed quite a bit since the PR was created. Merging it in after the refactoring is probably not a trivial task |
Given the number of data-loss issues encountered by people using Reminders with Tasks, at the moment I feel I wouldn't want to encourage Tasks users to use Reminders... See the warnings here, and multiple comments on the linked issue: |
I see, I think task notifications are a really key aspect of task management. I'm sure a lot of the issues from the reminders plugin comes from Tasks integration being a secondary feature and not the main consideration when creating the original plugin. I'm thinking of recreating the feature set in Tasks. Are there any dev docs published some where? Skimming through some of the PR, I saw that there is a tasks apiV1. If I end up finding the time to create a reminders like feature would it be possible to having something natively integrated or is using the apiV1 through a separate plugin the preferred way to interact with tasks. |
I do think that is very likely to be true.
Interesting. If you decide to go ahead, it would be a good idea for us to have a chat before you start work on it, to consider approaches that you might take. It would save us both a lot of time - you from having to figure stuff out, and me from reviewing code where perhaps I could have given you pointers from the outset, and then you from potentially reworking stuff! 😄 We would also talk about how it could be written with automated tests. My email is my profile here.
https://publish.obsidian.md/tasks-contributing/
I don't have enough to be able to talk about that. That would be a good thing to add to the list of things to discuss directly! 😃 |
Now I think I understand. Don’t worry about the API. Just think about the feature from the perspective of a Tasks user. |
One thing I forgot to say earlier. We must be mindful of respecting the license of any ideas or code from Reminders. Both plugins are MIT so the licence is compatible. But we would need to be sure that any terms like giving credit or whatever are respected. |
Perfect, I've created a new issue to get some input on the overall design aspect #1854 well reach out via email for my thoughts on the code implementation |
I love this plugin, I've been using it for a long time. It has almost everything that I wanted from obsidian (I came from a Notion environment), however, I wanted to have reminders for my tasks.
Using the workaround described here, I decided to modify the modal when creating/editing task with an option to add a reminder from obsidian-reminder plugin.
It uses the alarm clock emoji, the panel now looks like this:
The created task looks like this:
And the reminders plugin automatically recognizes the task as reminder:
As not everybody uses obsidian-reminders, I added an option to hide the reminders input in the create/edit task modal. It is hidden by default:
If it is false, it leaves the modal as before.
Thanks for this awesome plugin, I hope you like this.