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

Feature: Import from Google Keep #2015

Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from

Conversation

mgunyho
Copy link

@mgunyho mgunyho commented Apr 19, 2020

Fix

This PR adds the option to import notes from Google Keep. Notes can be either imported from a zip file exported from Google Takeout, or as individual JSON files. Attached is a small test file that contains notes with various features (check boxes, tags, etc.).

A couple of things to note:

  • Images and other attachments are not imported. I played around a bit with embedding the images as base64 in the markdown, but that is maybe not a very good idea with large images. It's also a bit of a hassle with the zipped files.
  • Archived notes get imported as just regular notes. I think the best way to handle this would be to add an option to the modal (next to 'enable markdown on all notes'), where the user could choose to move archived notes to the trash, add an user-specified tag or to import them regularly. I'm not familiar enough with react/redux to implement this myself.
  • It would be cool to parse links in notes and enable markdown on them (using e.g. linkify), not sure if it's worth it to add another dependency. The notes also have the annotations attribute that could be used for this.

I'm fairly new to typescript, so let me know if something looks wrong in the code!

Test

  1. Go to File-> Import Notes -> Google Keep
  2. Drag and drop e.g. this test zip file
  3. Observe that the notes have been imported

Release

RELEASE-NOTES.txt was updated with:

  • Added possibility to import notes from Google Keep

@dmsnell
Copy link
Member

dmsnell commented Apr 20, 2020

@mgunyho this looks great and it's so cool that you added it. we're a bit busy preparing the 1.16 release but I'd love to prioritize your PR. Please ping us again here if we haven't done more review by Monday.

@mgunyho
Copy link
Author

mgunyho commented Apr 29, 2020

Hi, any update on this?

@dmsnell
Copy link
Member

dmsnell commented Apr 29, 2020

Hi, any update on this?

Very nice @mgunyho you held me accountable 😄

Sadly no, we got a bit more distracted by some issues we uncovered while testing the 1.16 release. Those are mostly done. I will make sure to get you some review by the end of this weekend - setting an alert on my calendar to remind me.

Thanks for your patience! We appreciate your contribution so much and are excited to move it forward!


const title = importedNote.title;
let textContent = title ? title + '\n\n' : '';
textContent += get(importedNote, 'textContent', '');
Copy link
Member

@dmsnell dmsnell May 4, 2020

Choose a reason for hiding this comment

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

this is a minor style issue, but we don't need to mutate this variable when we can instead set it in one go

const title = importedNote.title;
const importedContent = importedNote.textContent ?? '';
const textContent = title ? `${ title }\n\n${ importedContent }` : importedContent;

Copy link
Author

@mgunyho mgunyho May 5, 2020

Choose a reason for hiding this comment

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

I removed the mutable variable in bd9425b, do you think the code is readable?

Side note: is the ?? operator widely enough supported? With a quick glance, I couldn't find it used elsewhere in this repo.

pinned: importedNote.isPinned,
tags: get(importedNote, 'labels', []).map(item => item.name),
},
{ ...this.options, isTrashed: importedNote.isTrashed }
Copy link
Member

Choose a reason for hiding this comment

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

if it were me I might suggest sending archived notes to the trash or add a tag to them - archived and leave them in the inbox.

trash is generally safe but someone could accidentally "empty the trash" and wipe out their archive

if, on the other hand, they import their archive into the "All Notes" section then they might get more notes in the list than they expect.

maybe we just need a setting…

Archived Notes: [ ] Import into trash [ ] Import with tag _________

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is how I would do it as well. I don't know how to add a (importer-specific) setting to the dialog.

Copy link
Member

Choose a reason for hiding this comment

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

I will try to play around with it this week and see if it wouldn't be too hard. if not, maybe adding archive to the tags could be a good interim solution?

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

This is looking great. I left some feedback on things that might be good to update. I'm sure our existing importers aren't the best examples of this, but it looks like we pass over a number of different errors/exceptions/failures in this PR and it would be nice to either handle those explicitly or add a comment in the code explaining why we're not and what we expect to happen - skip note vs. emit an error etc…

Thanks for your patience with this and thanks so much for your efforts! ❤️

? importedNote.listContent // Note has checkboxes, no text content
.map(item => `- [${item.isChecked ? 'x' : ' '}] ${item.text}`)
.join('\n')
: importedNote.textContent;
Copy link
Member

Choose a reason for hiding this comment

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

interesting. a note can only either be a list or some text?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, in Keep, you can "show checkboxes", which turns the whole note into a checklist, and as far as I can tell there's no way to have regular text then.

@KnifeFed
Copy link

Any updates?

@dmsnell
Copy link
Member

dmsnell commented Feb 14, 2022

@KnifeFed if there were updates you'd see them here, so no, unfortunately there are none.

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.

3 participants