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

Add open daily note base feature #79

Merged
merged 2 commits into from
Jul 15, 2020
Merged

Add open daily note base feature #79

merged 2 commits into from
Jul 15, 2020

Conversation

francishamel
Copy link
Contributor

@francishamel francishamel commented Jul 8, 2020

Open Daily Note is a core feature of roam. That's why this feature is considered as a core feature in foam's roadmap. This feature is quite simple. With the Foam: Open Daily Note command, vscode will create today's note (yyyy-mm-dd.md) if it's not already created and focus the editor on today's note. It is also possible to trigger the command via the Alt+d keyboard shortcut.

Some things related to the daily note are configurable:

  • Filename format
  • File extension
  • Title format of the daily note
  • Daily notes directory

This PR implements the Must Have and the Should Have.

Related issue: #54

Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for this addition @francishamel 🎉

@francishamel francishamel requested a review from jsjoeio July 9, 2020 18:58
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

awesome work @francishamel 🎉! this looks great. I tested it out locally and all works as expected.

there are some formatting changes I would make but we can always fix that later. i'll let you make that call.

suggested follow-up PRs:

  • update the official docs with info on how to use new features
  • add unit tests for the functions these new functions

No obligation to do these! Please let me know though. If you don't do it, I'd like to open a separate to keep track and open it up to others.

@francishamel
Copy link
Contributor Author

awesome work @francishamel 🎉! this looks great. I tested it out locally and all works as expected.

there are some formatting changes I would make but we can always fix that later. i'll let you make that call.

suggested follow-up PRs:

  • update the official docs with info on how to use new features
  • add unit tests for the functions these new functions

No obligation to do these! Please let me know though. If you don't do it, I'd like to open a separate to keep track and open it up to others.

For the docs, I'd be happy to update it in another PR. I'd just like some directions so that I don't miss anything. For the unit tests, I talked with @jevakallio and there isn't a test infrastructure yet in this project and I'm not that familiar with JS test frameworks to be honest. I'd be happy to learn but I don't feel like I'd be the best to add a basic test infrastructure tho.

@jsjoeio
Copy link
Contributor

jsjoeio commented Jul 10, 2020

For the docs, I'd be happy to update it in another PR. I'd just like some directions so that I don't miss anything.

Awesome! Why don't we move this discussion to an issue so it doesn't get lost? I'll add some thoughts to kick it off. Issue: #87

I'm not that familiar with JS test frameworks to be honest. I'd be happy to learn but I don't feel like I'd be the best to add a basic test infrastructure tho.

No worries there! Looks like @jevakallio mentioned it in #57 so we won't create a separate issue. If you want to learn, we can discuss there.

}

async function focusDailyNote(dailyNotePath: string) {
const document = await workspace.openTextDocument(Uri.parse(dailyNotePath));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future improvement (not expected now): Automatically move cursor to the end of file so you can just start typing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea! Opened up #118 to capture this.

Copy link
Collaborator

@jevakallio jevakallio left a comment

Choose a reason for hiding this comment

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

@francishamel this is looking good! I tested it with different permutations of settings, and it worked as expected! Can't wait to use this 🎉

I left a few minor nits in the comments, the biggest being that in preparation of landing #83, we should move the new code (apart from command registration) out into its separate file.

Francis Hamel added 2 commits July 14, 2020 21:57
- Activate command
- Add default keybindings for the command
- Use settings to configure the filename format, the file extension,
the title format and the directory where the note should be created
@jsjoeio
Copy link
Contributor

jsjoeio commented Jul 15, 2020

@francishamel ping me when this is ready again (here or on Discord) and we'll get this merged in!

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

looking good!

Copy link
Collaborator

@jevakallio jevakallio left a comment

Choose a reason for hiding this comment

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

Amazing, thank you so much @francishamel! ❤️

Thanks @jsjoeio @riccardoferretti for reviews 👍

@jevakallio jevakallio merged commit 6b99a8b into foambubble:master Jul 15, 2020
@jevakallio
Copy link
Collaborator

Landed, will release in next version!

@all-contributors add @francishamel for code ⌨️

@allcontributors
Copy link
Contributor

@jevakallio

I've put up a pull request to add @francishamel! 🎉

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.

4 participants