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: item list on new collection page #5039

Merged
merged 16 commits into from
Feb 27, 2023
Merged

feat: item list on new collection page #5039

merged 16 commits into from
Feb 27, 2023

Conversation

preschian
Copy link
Member

@preschian preschian commented Feb 18, 2023

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

Screenshot 2023-02-18 at 13 28 41

@preschian preschian requested a review from a team as a code owner February 18, 2023 06:30
@preschian preschian requested review from roiLeo and Jarsen136 and removed request for a team February 18, 2023 06:30
@netlify
Copy link

netlify bot commented Feb 18, 2023

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit cf5c790
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63fc8f414ccb3f000846af3a
😎 Deploy Preview https://deploy-preview-5039--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

maybe we should export some composable from useItemsGrid, I think we'll need to reuse useSearchParams in profile page & collection

components/items/Items.vue Outdated Show resolved Hide resolved
components/explore/ExploreTabItem.vue Outdated Show resolved Hide resolved
components/explore/ExploreTabItem.vue Outdated Show resolved Hide resolved
@vikiival vikiival mentioned this pull request Feb 20, 2023
components/explore/ExploreTabItem.vue Show resolved Hide resolved
components/items/ItemsGrid/ItemsGridImage.vue Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Co-authored-by: Viki Val <viktorko99@gmail.com>
@preschian
Copy link
Member Author

preschian commented Feb 20, 2023

I think we'll need to reuse useSearchParams in profile page & collection

actually, I already use <Item /> component on 3 pages

I just add this query on collection or user page:

hopefully, it helps us to simplify the code. wdyt?

Copy link
Contributor

@roiLeo roiLeo 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

@exezbcz
Copy link
Member

exezbcz commented Feb 23, 2023

image

It looks weird on my end. otherwise oki

@preschian
Copy link
Member Author

image

It looks weird on my end. otherwise oki

let me check first, should be not like that

@preschian
Copy link
Member Author

@exezbcz please check again. if still error, share me the link

@preschian preschian mentioned this pull request Feb 23, 2023
17 tasks
@exezbcz
Copy link
Member

exezbcz commented Feb 23, 2023

something is still wrong, is this related to the cards having different height? (afaik there is an issue for this)
image

link: https://deploy-preview-5039--koda-nuxt.netlify.app/bsx/collection/1299595206?page=2&redesign=true

@preschian
Copy link
Member Author

preschian commented Feb 23, 2023

something is still wrong, is this related to the cards having different height? (afaik there is an issue for this) image

link: deploy-preview-5039--koda-nuxt.netlify.app/bsx/collection/1299595206?page=2&redesign=true

I will check in separate PR 👍
thanks for the link

@roiLeo
Copy link
Contributor

roiLeo commented Feb 23, 2023

I will check in separate PR 👍 thanks for the link

We already had this issue, fix is to wrap cards content with container class

ref #4935

@preschian
Copy link
Member Author

I will check in separate PR 👍 thanks for the link

We already had this issue, fix is to wrap cards content with container class

ref #4935

oh thanks, will update then 🙏🏻

@roiLeo
Copy link
Contributor

roiLeo commented Feb 23, 2023

Why do we have a full-width layout here compared to explore layout?
We need to unify user experience

@preschian
Copy link
Member Author

Why do we have a full-width layout here compared to explore layout? We need to unify user experience

waiting for this decision actually #4857
should I put wrapper for now?

@roiLeo
Copy link
Contributor

roiLeo commented Feb 23, 2023

waiting for this decision actually #4857 should I put wrapper for now?

For now I would just copy same layout as we use currently on explore page.
#4857 fix might come later since we could have potential side effects (didn't test responsive)

@preschian
Copy link
Member Author

something is still wrong, is this related to the cards having different height? (afaik there is an issue for this)

For now I would just copy same layout as we use currently on explore page. #4857 fix might come later since we could have potential side effects (didn't test responsive)

updated ✅

please note, this PR is refactoring and under useExperiment(), so expect some features still missing or not perfect. such as infinity scroll or grid selection

@preschian
Copy link
Member Author

@exezbcz @kodadot/internal-dev any feedback?

@daiagi daiagi self-requested a review February 24, 2023 11:27
@Jarsen136
Copy link
Contributor

Complexity 4

I just wonder if we could manage to reduce it

@preschian
Copy link
Member Author

preschian commented Feb 24, 2023

Complexity 4

I just wonder if we could manage to reduce it

for total lines code I think that is the tricky one. because composable is a function inside a function. for example code climate read useFetchSearch() instead of fetchSearch(page)

for cognitive complexity, let me try to fix it in this PR #5106

@exezbcz
Copy link
Member

exezbcz commented Feb 24, 2023

@preschian apart from this issue I think everything seems to be fine
#5039 (comment)

@preschian
Copy link
Member Author

can we merge this, please?

@daiagi daiagi added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Feb 27, 2023
@codeclimate
Copy link

codeclimate bot commented Feb 27, 2023

Code Climate has analyzed commit cf5c790 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4

View more on Code Climate.

@roiLeo roiLeo merged commit 6eff516 into main Feb 27, 2023
@roiLeo roiLeo deleted the refactor/item-list branch February 27, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants