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

Modal Creator for Bookmarks, Images, and Files #35

Merged
merged 20 commits into from
Oct 20, 2021
Merged

Conversation

ianconsolata
Copy link
Contributor

These do not show up in the dashboard UI yet (you can add them, they show up in the index, but everything that does not have a note is filtered out.

@vercel
Copy link

vercel bot commented Oct 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mysilio/garden/HAnhk7d9vJ7VjqAoyGQTyo9Ag33N
✅ Preview: https://garden-git-id-my-176.mysilio.page

@ianconsolata
Copy link
Contributor Author

@travis Why do you use nameForFile to generate a UUID filename rather than just uploading it using file.name? https://github.com/mysilio-co/garden/blob/main/components/ImageUploader.jsx#L55-L72. Should I use that in the File uploader too? Right now I am just using file.name. I kind of like it as it results in fairly readable filenames / paths, but I assume there is a reason you went with UUIDs so wanted to check in: https://ian.myunderstory.com/public/apps/understory/env/dev/files/DisCO_Manifesto-v.1.pdf

@travis
Copy link
Contributor

travis commented Oct 15, 2021

@travis Why do you use nameForFile to generate a UUID filename rather than just uploading it using file.name? https://github.com/mysilio-co/garden/blob/main/components/ImageUploader.jsx#L55-L72. Should I use that in the File uploader too? Right now I am just using file.name. I kind of like it as it results in fairly readable filenames / paths, but I assume there is a reason you went with UUIDs so wanted to check in: https://ian.myunderstory.com/public/apps/understory/env/dev/files/DisCO_Manifesto-v.1.pdf

we chatted in Slack but to memorialize - I think this was to avoid potential duplicate filenames. might not actually be an issue, and we could always append a unique id to the filename to get the best of both worlds

Copy link
Contributor

@travis travis left a comment

Choose a reason for hiding this comment

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

This all looks good! I'd like to start using that new model logic for note saving too, particularly in createOrUpdateConceptIndex:

export function createOrUpdateConceptIndex(

Right adding metadata to a concept requires adding it there so that it doesn't get lost on save, feels fragile! Definitely don't need to get this change in now, just flagging so we can sync on this data model stuff soon.

const onSubmit = async () => {
if (file) {
const fileUrl = `${fileContainerUri}${file.name}`;
console.log('url', fileUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove console log

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.

2 participants