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

feat: implement gallery drafts hook #315

Merged
merged 28 commits into from
Nov 2, 2023

Conversation

shahin-hq
Copy link
Contributor

@shahin-hq shahin-hq commented Oct 30, 2023

Summary

Closes: https://app.clickup.com/t/862kh238w

This PR implements a hook to save/update gallery drafts. It uses indexed-db under the hood to avoid potential storage limits of localStorage.

Check #326 to see the hook in action or testing

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ItsANameToo ItsANameToo added this to the 0.11.0 milestone Oct 30, 2023
@shahin-hq shahin-hq marked this pull request as ready for review October 31, 2023 11:34
return allDrafts.filter((draft) => draft.walletAddress === wallet?.address);
};

const setDraftCover = (image: ArrayBuffer | null, type: string | null): void => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to use ArrayBuffer to store images as Safari cannot store Blobs

If you are storing large, user-generated files such as images or videos, then you may try to store them as File or Blob objects. This will work on some platforms but fail on others. Safari on iOS, in particular, cannot store Blobs in IndexedDB.

Luckily it is not too difficult to convert a Blob into an ArrayBuffer, and vice versa. Storing ArrayBuffers in IndexedDB is very well supported.

Remember, however, that a Blob has a MIME type while an ArrayBuffer does not. You will need to store the type alongside the buffer in order to do the conversion correctly.

https://web.dev/articles/indexeddb-best-practices

@shahin-hq shahin-hq mentioned this pull request Oct 31, 2023
9 tasks
@ItsANameToo ItsANameToo mentioned this pull request Oct 31, 2023
9 tasks
Copy link
Contributor

@alfonsobries alfonsobries left a comment

Choose a reason for hiding this comment

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

code looks good but think we should add some tests, seems complex enough to fail with some minor adjustment

@goga-m goga-m marked this pull request as draft November 1, 2023 09:54
@alfonsobries
Copy link
Contributor

@shahin-hq maybe we should treat this one and #326 a single task with a single pr, testing this requires #326 anyway

@shahin-hq
Copy link
Contributor Author

@alfonsobries surely we can do that also note that I added unit tests for the hook, so it can be tested by reading unit tests.

@shahin-hq shahin-hq marked this pull request as ready for review November 1, 2023 17:52
@alfonsobries alfonsobries changed the base branch from develop to feat/gallery-drafts November 1, 2023 22:40
@alfonsobries alfonsobries changed the base branch from feat/gallery-drafts to develop November 1, 2023 22:40
@alfonsobries
Copy link
Contributor

@shahin-hq think this pr should be against feat/gallery-drafts

@alfonsobries alfonsobries marked this pull request as draft November 1, 2023 22:42
@shahin-hq shahin-hq changed the base branch from develop to feat/gallery-drafts November 2, 2023 10:19
# Conflicts:
#	resources/js/Components/Sidebar/SidebarItem.tsx
#	resources/js/I18n/Locales/en.json
#	resources/js/Pages/Settings/Layout.tsx
# Conflicts:
#	resources/js/Components/Sidebar/SidebarItem.tsx
#	resources/js/I18n/Locales/en.json
@@ -8,6 +8,7 @@ describe("SidebarItem", () => {
<SidebarItem
icon="Cog"
title="General"
href="/hello"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to fix that in order to make CI happy

@shahin-hq shahin-hq marked this pull request as ready for review November 2, 2023 13:10
Copy link
Contributor

@goga-m goga-m left a comment

Choose a reason for hiding this comment

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

LGTM, I don't see any issues other than those that @alfonsobries mentioned and what we discussed in thread. We could also continue reviewing the integration part in #326

@goga-m goga-m merged commit eb90aae into feat/gallery-drafts Nov 2, 2023
22 checks passed
@goga-m goga-m deleted the feat/persist-gallery-drafts branch November 2, 2023 15:13
@goga-m goga-m mentioned this pull request Nov 15, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants