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

refactor: create gallery NFTs #370

Merged
merged 20 commits into from
Nov 9, 2023
Merged

Conversation

shahin-hq
Copy link
Contributor

@shahin-hq shahin-hq commented Nov 6, 2023

Summary

https://app.clickup.com/t/86793qcn4

This PR refactors the useGalleryNfts hook to be responsible for fetching and searching NFTs. In short, I removed logic that fetches NFTs in backend and passes to the frontend components because the NFT slider doesn't really use the preloaded data. Removing backend fetching is a good idea because of the reasons below:

  • Makes page loads faster (let's postpone NFT fetching until we really have to)
  • It will make things a little bit simpler as we won't have to sync data: pipe backend data to frontend, instead frontend will be responsible for fetching and displaying data
  • As a next step we can utilize react-query to cache data - we don't have to fetch NFTs every time user opens the slider, we could cache and reuse

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

@shahin-hq shahin-hq marked this pull request as ready for review November 6, 2023 17:56
@@ -204,10 +198,6 @@ export const useGalleryNtfs = ({
};

const searchNfts = async (query?: string): Promise<void> => {
if (!isTruthy(pageMeta.first_page_url)) {
throw new Error("[searchNfts] First page url is not defined.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

first_page_url is always there, so we can remove this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing broken tests, nothing new.

app/Http/Controllers/MyGalleryController.php Outdated Show resolved Hide resolved
app/Http/Controllers/MyGalleryController.php Outdated Show resolved Hide resolved
@shahin-hq shahin-hq requested a review from crnkovic November 6, 2023 20:05
@ItsANameToo ItsANameToo added this to the 0.11.0 milestone Nov 7, 2023
@ItsANameToo
Copy link
Contributor

some conflicts in the meantime @shahin-hq

# Conflicts:
#	app/Http/Controllers/MyGalleryController.php
#	tests/App/Http/Controllers/MyGalleryControllerTest.php
@ItsANameToo ItsANameToo merged commit 8e2b7f1 into develop Nov 9, 2023
16 checks passed
@ItsANameToo ItsANameToo deleted the refactor/create-gallery-nfts branch November 9, 2023 10:14
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.

4 participants