-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add ability to open Trello card in desktop app #57
Add ability to open Trello card in desktop app #57
Conversation
932269f
to
c5e4375
Compare
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 pretty good! I think this is a great addition.
@@ -63,6 +64,20 @@ export class TrelloSettings extends PluginSettingTab { | |||
}); | |||
} | |||
|
|||
private openInDesktopSetting(containerEl: HTMLElement, settings: PluginSettings): void { | |||
if (Platform.isMacOS || Platform.isWin) { |
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'm fine with hiding this setting on other platforms, as long as we also override the saved settings on other platforms. I think a check in plugin.ts
after loading settings, but before initializing the state:
// Set up data and default data.
const savedData: PluginData | undefined = await this.loadData();
const data: PluginData = Object.assign({}, DEFAULT_DATA, savedData);
data.settings = Object.assign({}, DEFAULT_SETTINGS, savedData?.settings);
data.settings.customUi = Object.assign({}, DEFAULT_SETTINGS.customUi, savedData?.settings.customUi);
// Add something like this
if (!Platform.isWin && !Platform.isMacOS) {
data.settings.openInDesktop = false;
}
this.state = new PluginState(this, data);
It's not a big deal but I know I use a synced obsidian vault on multiple platforms. I wouldn't want someone to enable it on a Mac, then open Obsidian on Linux and have everything be broken.
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.
@nathonius OK, I will need to test this, and it's a busy week, leave it with me
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.
@nathonius Added, but I am finding it really hard to test, as everything still shows on iPad and actually still opens in the iPad Trello app anyway 🤷 . Do you have a way to test on Linux?
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.
Yeah I can test on a Linux desktop. Will do that in the next day or two. Maybe Android would work too? I can modify to allow the setting there and see if it opens the Trello app.
While you're in plugin.ts, I just noticed I spelled Obsidian wrong in the log function 🤣 log(context: string, message: string, logLevel: LogLevel = LogLevel.Info): void {
if (this.state.getSetting('verboseLogging')) {
const log = `OBISIDAN-TRELLO: (${context}) ${message}`; |
@nathonius I guess most people don't look… Fixed it in this PR. |
…dian-trello into chrischinch/desktop
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.
Let's get those few changes in here and we can get this merged and released. 😄
Co-authored-by: Nathan <[email protected]>
…dian-trello into chrischinch/desktop
Quality Gate passedIssues Measures |
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. I'm going to try testing on Linux but otherwise I think this is good for release.
As far as I can tell, there is only a Windows and macOS desktop application.