-
Notifications
You must be signed in to change notification settings - Fork 2
Introduce support for persistentNotifications #10
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
base: master
Are you sure you want to change the base?
Conversation
I've adapted all the tests, so they are passing. I am still seeing an error
which I would like to ask you, @semanticdata to correct. I don't know where to start looking. |
- Renamed variables to more readable values - Moved from manual timing to waiting for an epoch - Pause state managed by storing left-over duration in timer - running state inferred from ticker interval being set - Start / Pause managed in single toggleTimer place (you either pause, or unpause. no in-between)
There are still issues with floating point precision and "addDropdown" for some reason does not exist in Obsidian Settings mock
64837d3
to
5db6309
Compare
src/components/SettingsTab.ts
Outdated
value ? this.plugin.settings.persistentNotification = false : null; | ||
await this.plugin.saveSettings(); | ||
this.display(); |
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 fun.
Toggles the persistentNotification setting off, when autoProgress is enabled. Neither option can function with the other enabled, so... this is a nice alternative to dropdown selector.
jest.mock('obsidian'); // This will use the __mocks__/obsidian.ts automatically | ||
jest.mock('obsidian', () => { | ||
const obsidian = jest.requireActual('obsidian'); | ||
const moment = jest.requireActual('moment'); | ||
|
||
return { | ||
...obsidian, | ||
moment | ||
} | ||
}); // This will use the __mocks__/obsidian.ts automatically |
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 was required for moment
to work with the tests.
It might be a reason why the Settings mock breaks and we get an error with .addDropdown()
Hello, sorry I've been busy at work recently. I'll take a look at this as soon as I get a chance. Thank you very much for taking the time to collaborate. |
No pressure @semanticdata I have been actually using the timer, with persistent notification & enjoyed it. Only issues being:
There is still alot that can be done better. And, I am going to be gone from programming, for at least a month now. So, no pressure whatsoever. |
moment.abs()
, it is broken. Doesn't just return the value, it overrides the object it is ran on...)moment
lib usable with jestmoment.duration().toISOString()
(.seconds()
has the same issue).addDropdown()
availableHello there @semanticdata
I had tried your plugin a few months ago and had in the back of my head, that I would like to see persistentNotifications in it. Hence, this PR.
I've got to say... the amount of tests in the code is ASTRONOMICAL. There are more tests than there is actual code 😅
I don't think it is something I am going to be able to figure out on my own. Help and suggestions would be greatly appreciated.