Skip to content
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

Commands for opening notes in a vertical split #166

Merged
merged 7 commits into from
Aug 3, 2023

Conversation

stefanlogue
Copy link
Contributor

Added some commands to open notes in a vertical split:

  • ObsidianTodayVertical
  • ObsidianYesterdayVertical
  • ObsidianNewVertical

Also updated README.md to reflect changes.

Closes #165

@epwalsh
Copy link
Owner

epwalsh commented Aug 3, 2023

Hey @stefanlogue, thanks for this. I would like to keep the overall number of commands to a minimum though, so IMO a better option would be to add an argument to the existing commands which determines how to open those buffers (vertical, horizontal, or replace current buffer). What do you think?

@stefanlogue
Copy link
Contributor Author

Yeah that sounds like a better implementation, I can have a look at implementing that tomorrow/early next week. Should I close this PR and open a fresh one, as technically it's a different feature set?

@epwalsh
Copy link
Owner

epwalsh commented Aug 3, 2023

@stefanlogue up to you! Fine with me if you just make those changes in this PR.

@stefanlogue
Copy link
Contributor Author

I think this could become quite tricky in the case of new notes, which already have an optional arg for the name of the note. If a user doesn't want to name their note, but does want to open it in a split, how do we discern that the passed argument is supposed to target the opening strategy rather than the name?

Your input on this would be much appreciated, as I want to avoid breaking any existing patterns/introducing any breaking changes as much as possible

@epwalsh
Copy link
Owner

epwalsh commented Aug 3, 2023

Good point.

how do we discern that the passed argument is supposed to target the opening strategy rather than the name?

Another option would be to have a config setting that determines how to open notes for all of those commands.

@stefanlogue
Copy link
Contributor Author

Is opening followed links in splits within the scope of this, or is that functionality you'd rather not have?

@stefanlogue
Copy link
Contributor Author

I've pushed changes to match the implementation above, and decided that opening links in a split is a possible future feature but not needed at this time

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @stefanlogue :)

@epwalsh epwalsh merged commit c396640 into epwalsh:main Aug 3, 2023
5 checks passed
if data.args:len() > 0 then
note = client:new_note(data.args)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the change from data.args to data.args[1] has just broken the functionality of the :ObsidianNew command. If I call :ObsidianNew Example, then:
data.args = "Example"
data.args[1] = nil
which breaks note_id_func since it never recieves the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a remnant of my first pass when I tried to do this via additional arguments, must have missed this change, will open a PR for a fix

flaviosakakibara pushed a commit to flaviosakakibara/obsidian.nvim that referenced this pull request Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Open notes in a vertical split
3 participants