Add Advanced Recurring Rules to the Event Create/Edit Event modal #500
Conversation
…rk cross-platform.
|
Hey! this is exactly what I was looking for. |
|
Just to clarify, are you asking if there is a way you can get these changes in your local obsidian? If that is the case, then if you clone the project locally, run As with any time that you manually install things like this, I would probably suggest taking a backup of your vault just to be sure, although the changes here shouldn't cause any problems. The current recurring events are the only ones that will get updated, and only when they are manually opened/saved. Beyond that, it is just adding new recurring events that should be affected by these changes. |
|
I tested this on my iPhone SE 2020 (iOS 16.6) with no issues. I tried a variety of options including every n weeks, every n days, every n months, and every n years, all of which worked as I expected them to. Great work! |
|
If anyone wants to test this pull request without needing to clone down and build, below is a package of the generated plugin files I obtained by building on my machine. You can use this by extracting to the ".obsidian/plugins/obsidian-full-calendar" folder. As @bskeen suggested, I also suggest backing up your vault or creating a new test vault when testing plugins. That way you don't risk ruining your primary one. |
|
This seem to work well 👍 |
|
This is great, thanks for the awesome work @bskeen! I am posting this from my phone right now but will try and get a full review up in the next day or two. As a more meta-point, I'll be trying to work through the backlog of pull requests and new bug reports over the next few weeks. |
…ther repeating controls.
|
Thanks, @davish! Maybe once this is merged, I could work on the other features I mentioned, such as excluding dates or custom rules. |
There was a problem hiding this comment.
This is really great work, thanks for the PR! My main request is for you to add some more documentation -- I had some trouble tracing through event-recurrence-types.ts and its associated tests, and documentation will make it easier for me and for other contributors to understand this code going forward. Thanks again for you contribution!
| @@ -0,0 +1,402 @@ | |||
| import { DateTime } from "luxon"; | |||
There was a problem hiding this comment.
Could you add some explanatory comments in this file? The logic here seems pretty involved, and for future reference it would be good to have some explanations along with this code.
There was a problem hiding this comment.
I added some explanations to this file. Please take a look and see if there is anything else you would like me to document.
| return `${formatOrdinalNumber(value)} to last`; | ||
| }; | ||
|
|
||
| export interface RecurrenceInfo { |
There was a problem hiding this comment.
Specifically, could you leave a comment explaining what each of these functions is used for?
There was a problem hiding this comment.
I added some explanations to this file. Please take a look and see if there is anything else you would like me to document.
| getDisplay: (dateStats: DateStats) => string; | ||
| } | ||
|
|
||
| export const MONTH_RECURRENCE_INFO: RecurrenceInfo[] = [ |
There was a problem hiding this comment.
I'm also curious: how is the recurrence type selected?
There was a problem hiding this comment.
The recurrence type is selected inside the MonthYearSelect component, which is in the EditEventRecurrence.tsx file, at the bottom. That provides a select box that is populated with either the monthly or yearly options.
There was a problem hiding this comment.
Rereading this question, I think you could also mean how is it selected when it is coming in to the component? The answer to that is it looks through the different RecurrenceInfos for the current frequency, and it calls the hasProps function on each until one returns true. If they all return false, then it selects the first by default.
| }); | ||
|
|
||
| describe("when day is a positive number", () => { | ||
| beforeEach(() => { |
There was a problem hiding this comment.
Just curious: Why write a beforeEach() callback when there is only one test case in this describe block?
There was a problem hiding this comment.
I'll take responsibility for this as he wrote this according to my testing style. Basically, since the describe block defines the condition, semantically I think it makes sense to have all the code connected to that condition in a place where no matter how many tests are in that describe context, it is guaranteed to have that condition met. This leaves the it statements as one-liners reserved for assertions only. In this case (and admittedly many cases), where you only need to assert one thing, this perhaps becomes more verbose than strictly required; however, it does have the benefit of if you ever need multiple assertions in the future, you can add more it blocks and not sacrifice the clear output in case of test failure.
At the end of the day, it is a style choice, just as much as using curly braces for the body of a one-line if statement.
For more information on my unit testing philosophy, you can check my talks on the matter (I need to sit down and document it in textual/reference form one of these days).
Conf42 Rustlang 2023: https://youtu.be/-fDIPnG6xJc?si=VA3nY3c2cTDrzZtM (my actual talk, in Rust, but the ideas translate over)
Rendercon Kenya 2023: https://world-class-engineers.github.io/rendercon-2023-unit-testing/ (this is my slides, but the recording isn't available yet)
There was a problem hiding this comment.
Just to add to what @joeskeen said, I did follow this style; however, I do know that I am contributing to someone else's repository, and so I would be happy to change that if you would like. I think consistency is important inside any project.
…s more clear what everything there is doing.
|
Hey @davish I submitted some changes for your feedback, and answered the questions you had. Please let me know if I answered them well enough, or if you need me to change anything else. Thanks again for the great plugin! |
|
@davish I just downloaded Full Calendar and bskeen's work would help out alot, can't keep track of birthdays on my own!! I hope it gets merged soon! |
|
@davish Just checking in since there hasn't been movement on this in awhile. Is there anything else you need me to fix or add to this PR? Just anxious to begin using it myself. |
|
Hi @bskeen, apologies for my absence from this thread -- life has gotten in the way of open source maintenance recently. The one thing that I would like to see before merging this PR would be to refactor the tests to have them rely less on mutable state between multiple tests in the same suite. It's very difficult for me to follow what the tests are actually asserting, and I think it would make it hard for me to maintain this in the future if any of the tests start failing. |
|
@davish I finally went back and updated the tests. I removed the shared variables (although I left a few constants, but those don't change between the tests that use them). I also split the large test file into one for each of the different recurrence types. Please take a look and let me know what you think. |
|
@davish Just thought I would check-in since it has been a few weeks. It looks like the repo has moved since I opened this PR, are you still the right one to message about this pull request? |
|
@chrisgrieser @joethei @konhi @phibr0 It seems like you four are the only ones in the obsidian-community organization (where this repo is now located). This PR is for a highly requested feature for the Full Calendar plugin, and I was wondering if one of you might be the right one to talk to about this? I was able to update this in the way requested a couple months ago and I was just wondering if there were any chance we could get this merged if possible. |
|
@chrisgrieser @joeskeen @joethei @phibr0 @davish |
|
Amazing stuff!! The only bug I found was creating event titles that were solely numbers (123 for example). |
|
Good job with this man!! I love it |
|
Please approve the merge!! |
I added some UI to the Create and Edit Event Modal to handle advanced recurring events, using the already included RRule library and functionality that was already built into this plugin. Included in these changes:
recurringtype to therruletype when the event is opened to edit.Please take a look and let me know what you think. I thought this would at least be a good starting point, since this seems to be a highly-requested feature.
NOTE: Support is not yet added in the PR for excluding dates from the rule, but should be fairly easy to add in next. Another feature mentioned in the discussion is the ability to add in your own custom text that will be rendered as a rule. This would also be pretty straightforward to add. I mostly wanted to get something we can start from out there if possible, and then we can improve it going forward.