-
Notifications
You must be signed in to change notification settings - Fork 107
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
Always open in new tab setting #497
base: main
Are you sure you want to change the base?
Always open in new tab setting #497
Conversation
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.
Thank you for your contribution! Just a few suggestions before merging.
src/ui/settings.ts
Outdated
@@ -27,6 +27,8 @@ export interface FullCalendarSettings { | |||
}; | |||
timeFormat24h: boolean; | |||
clickToCreateEventFromMonthView: boolean; | |||
alwaysOpenInNewTab: boolean; |
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.
alwaysOpenInNewTab: boolean; | |
alwaysOpenInNewTab?: boolean; |
Both of these settings should be optional, since they won't be present in existing Full Calendar installations. Following the type errors should lead you to add some more edge cases to fill in the defaults as they should in those cases.
src/ui/actions.ts
Outdated
* @returns | ||
*/ | ||
export async function openFileForEvent( | ||
cache: EventCache, | ||
{ workspace, vault }: { workspace: Workspace; vault: Vault }, | ||
id: string | ||
id: string, | ||
settings?: FullCalendarSettings |
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.
settings?: FullCalendarSettings | |
openInNewTab: boolean |
Could you pass the setting in as its own parameters rather than passing in the whole settings object? I'd prefer it to be explicit what settings openFileForEvent
is using from its type signature.
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.
Sure, just updated the branch
I prefer the calendar tab not to close when opening an event (but open a new tab instead). This way its easier to compare and switch notes of multiple events. So maybe we could add this setting to always have clicks to open in a new tab?
Without setting (current, many clicks needed)
With setting (this PR, less clicks needed)
Invert Ctrl-setting for opening modal vs file
Also (second commit in this PR), I tend to want to open the note more than I want to open the edit modal. So I added a setting for inverting the modifier key as well (ctrl for opening modal instead of ctrl for opening the file - notes are central for me):